added Units for AtomsBase interface + a Base.show routine

[JanHab]

Hello - I added Units for the box and positions.

In AtomsBase the user can use any length unit and is not forced to use Ängstrom. Therefore I added a _write_convert routine specified for Length units which converts the units in Ängstrom and then uses ustrip. to cut the unit.
I would like to add this for units like mass and velocity, if this is wished by you, but could not find the units ExtXYZ uses in this cases. At the moment these units are just cutted of.

Additionally I added that the atomic_masses are looked up in the Atoms function and are deleted from atom_data in the write_dict function so that they are not written in the final file.
Is this wished or should they be written down in the file as well?

[jameskermode]

Thanks, this is nice. I agree it would be good to add unit conversion for velocities and masses. ExtXYZ spec doesn’t impose a choice of time units, but let’s follow the ASE units system:

https://wiki.fysik.dtu.dk/ase/ase/units.html

which means lengths are in A, energies in eV and masses are in amu and time thus has somewhat strange units.

[jameskermode]

Regarding the atomic_masses property, I agree with removing them from the dict to skip writing but only if they were automatically created from chemical species. If the input file had a custom mass property this should be persevered in a read / write / read round trip. Not sure how best to implement that.

[jameskermode]

Please could you also add a test of the unit conversion functionally?

[JanHab]

atomic_masses:
What do you think about getting the atomic_mass of the atom type with PeriodicTable and comparing this to the atomic_mass of the atom. If both are not the same the atomic_masses are kept and written in the file.
If the input file has atomic_masses aren't they already read and saved as atomic_masses in atom_data?

unit test:
Isn't his already tested in the AtomsBase testset? There is read the infile (seq1), saved in a new file and read again (seq2). Both are tested if they are the same. If the unit conversion would fail, wouldn't we get a failed test there?
I can add an extra test were it is tested if the unit is Ängstrom e.g. if you would like to have this?

[jameskermode]

Both solutions sound good including the extra test of unit conversion, please go ahead.

[JanHab]

atomic_masses:
Now the atomic_mass is kept if this differs to the atomic_mass got by PeriodicTable.
If the input file got a custom mass property this atomic_masses are read with the unit "u".

unit test:
I added a unit test for position, bounding_box and atomic_mass.
Just the position, bounding_box and atomic_mass are read with units yet. However, energies and other properties are read without any unit.
I am not sure if this should be changed.
I could add units for energies like dft_energy, but i am not sure if there usually occur a lot of different energies in .extxyz files?
This would lead to a lot of extra cases to add units to all of these energy types.
They still would be converted in "eV" when a system is saved to a file.

What are your thoughts on this?

[jameskermode]

I think also converting units for the ASE default property names of momenta (from u*A/(A*sqrt(u/eV))), energy and energies (from eV), forces (from eV/A), stress and stresses (from eV/A^3) and virial (eV) would also be nice, but as you point out there could often by multiple energies, forces etc contained within an extxyz file, and I don't think it's sensible to try to infer the users' intentions from their choice of name.

[JanHab]

Okay, now i have added those units, hopefully i don't miss any unit?
I added test routines for all of those units where i create a system, save this, load it again and check that the unit conversion was right. This seems to work.

For velocities I have added that they also get a unit by reading, but are not set on zero by default, as you wished.

I have added the Base.show routine with the name "Atoms" for the system. There i am not sure how to create a test for this, any ideas?

The data routine I have skipped for the moment, I think i understood something wrong about the data field in AtomsBase. I will think about how to do this the best. In AtomsBase there is no way to get all this information, saved in the data block of FlexibleSystem or Atom, yet, but i think it would be nice to add this in both, AtomsBase and here.

What are your thoughts on this? Happy about any feedback or further ideas.

[jameskermode]

I have added the Base.show routine with the name "Atoms" for the system. There i am not sure how to create a test for this, any ideas?

Could the string representation could be evaluated as Julia code and then checked for equality with the original struct?

Happy to leave off the data() method from this PR. I agree that's a current limitation in AtomsBase.

[JanHab]

Okay I have added a test for the show representation.
Not sure if this is exactly what you meant, but it tests if the representation of seq1 is equal to the representation of seq2.
It works with a String comparison.

[jameskermode]

Looking good generally, just a couple of minor comments

[jameskermode]

Thanks for all the hard work. I'll go ahead and merge and then tag a new release. We can pick up any further things in new PR(s).

[JanHab]

Cool, thanks a lot.

[jameskermode]

@JanHab v0.1.7 has been released via General registry