ASE: Use guess=true while checking parser support

[rashidrafeek]

Is there a reason for setting guess=false while checking parser support? Only if guess=true will ASE guess the extension based on extension and therefore can read many formats like VASP POSCARS (*.vasp, POSCAR*), QE input (*.in) etc. Currently, it shows an error: Python: UnknownFileTypeError: Could not guess file type due to setting guess=false.

[mfherbst]

Thanks for the suggestion.

Hmm I guess that was to be careful and to avoid issues when file extensions are not unique. For sure we should make this a parameter exposed to the user (eg a flag in ASEParser, but before changing it we should double check this has no negative side effects.

We could also have two ase parser objects in the default list of parsers: the first more strict and the second laxer. This would make it more obvious what's the cause in case sth. breaks in the future.

[mfherbst]

Oh also please provide example files for edge cases / unexpected behaviour if you can. Always good to add them to the atomsio test suite, because the whole parsing stuff is indeed quite messy.

[rashidrafeek]

Sorry for the late reply. I was a little busy and could not respond to you comments.

I think defining these parameters in the AseParser object with a default value using @kwdef would be better instead of defining multiple parser objects. For example, we can redefine AseParser as,

@kwdef struct AseParser <: AbstractParser 
    read::Bool = false
    guess::Bool = false
end

function AtomsIO.supports_parsing(parser::AseParser, file; save, trajectory)
    ...
        format = ase.io.formats.filetype(file; read=parser.read, guess=parser.guess)
    ...
end

Additionally, it seems that currently the read argument above (which enables reading the file to determine filetype) is mapped to the save argument of supports_parsing. AFAIU, these two flags are independent and mapping them leads to incorrect behavior. For example, even though ASE can both read and write "*.xsf" files, currently it gives an error when save=false:

julia> using AtomsIO: supports_parsing
       using AtomsIOPython: AseParser

julia> supports_parsing(AseParser(), "test.xsf"; save=true, trajectory=false)
true

julia> supports_parsing(AseParser(), "test.xsf"; save=false, trajectory=false)
ERROR: UndefVarError: `PyException` not defined
Stacktrace:
 [1] supports_parsing(::AseParser, file::String; save::Bool, trajectory::Bool)
   @ AtomsIOPython ~/GitProjects/AtomsIO/lib/AtomsIOPython/src/ase.jl:22
 [2] top-level scope
   @ REPL[46]:1

caused by: Python: UnknownFileTypeError: Could not guess file type
...

This behavior is present for "*.vasp" files also.

There are individual functions can_read and can_write in the IOFormat object of ASE to determine whether ASE supports reading/writing for a specific format. I think these should be used instead. To change this, the following lines:

AtomsIO.jl/lib/AtomsIOPython/src/ase.jl

Lines 32 to 34 in f15cec6

	else
	return true
	end

would need to changed to

    ioformat = ase.io.formats.ioformats[format]
    elseif save
        return ioformat.can_write
    else
        return ioformat.can_read
    end

If these changes seem fine, I will modify the PR accordingly. Let me know what you think.

[mfherbst]

Sounds good. I'm a bit confused what the read ASE kwarg is then actually for. Is there no way we can give it a sane default and not worry about it from then on?

[mfherbst]

Thanks getting close. Just a small refactor ;)

[rashidrafeek]

I have implemented your suggestions. And have removed the read argument from the AseParser for now as you suggested. The issue with the read argument would not occur if guess=true is made the default. Wouldn't this default make more sense as this is the current default in ASE?

I have also added a test for xsf format which would not pass in the current master. Let me know if these changes are okay.

[mfherbst]

@rashidrafeek Could you add tests in lib/AtomsIOPython/test/ase.jl for the cases you would want to have supported, just to ensure that we don't break things in the future ?

[rashidrafeek]

Sorry for the delay here. I have added .xyz and .vasp to the tests, which I use regularly.

Also, there was a mistake in the wording of the warning. The bug occurs when trying to read, not write (i.e, save=false). Hope it is good to go now.

[mfherbst]

Thanks!