jameskermode
commented
1 year ago Thanks @mfherbst for identifying the problem. I agree stripping units is problematic. I’d be very open to a PR to fix these unit issues along similar lines to what you’ve done in AtomsIO, or even to removing the generic Atoms type from this package entirely and using the one exposed by AtomsIO instead in downstream packages.
I originally intended this AtomsBase interface to be a draft and mainly merged it into the main branch of ExtXYZ at your suggestion 😀
When integrating ExtXYZ with AtomsIO I found a bunch of issues in the AtomsBase interface, which currently lead me to rewrite most of it in AtomsIO, see https://github.com/mfherbst/AtomsIO.jl/blob/master/src/extxyz.jl. Of course better this should be integrated here.
One concern for me at the moment are the lines https://github.com/libAtoms/ExtXYZ.jl/blob/2c35dcb51728dd2e458807332a2c4fe27c31c8bb/src/atoms.jl#L53--L66 which effectively strip off all units of all data stored in the
ExtXYZ.Atoms. I think this is problematic, because it is later impossible to recover the unit associated to a key. I think it should be the responsibility of the user to do the intrusive stripping off of the units and rather just warn about unsupported unitful quantities and ignore them. My rationale would be that it is better to ignore a key rather than conveying information, which could be misleading accidentality. This is the approach I have followed in AtomsIO. Essentially I only support unitless quantities or unitful quantities, which follow a convention, which is documented in AtomsBase (see this PR).@jameskermode What is your opinion about this?