@richardjgowers wrote these readers IIRC. I have no experience with them, but if they specify their formats then we better adhere to them!

We might have to issue a warning with these readers for at least one release cycle, saying that they now convert!

So I hacked in writing ibisco force to the output, so I'm not sure that there's anyone else creating these files... And I can't remember what units these are in.

Not sure about the dl poly units, if they are specified we should probably convert? Tbh a better solution would be to keep track of units (maybe even in something like an xarray array) and do conversion properly (peek into astropy and see how they handle it?)

So I hacked in writing ibisco force to the output, so I'm not sure that there's anyone else creating these files... And I can't remember what units these are in.

How can we find out? Are there any docs?

Or if this is an MDA extension, then we need to document it here and whatever we say is the rule.

Or we drop it.

Not sure about the dl poly units, if they are specified we should probably convert?

Yes! — Are there docs for the file formats available somewhere? EDIT: sorry, DL_Poly docs were linked in the post...

Most file formats that we encounter come with defined units, so the units are part of the file format definition (the semantics). We should always be able to convert to our unit system and convert back without having to track units. (I imagine that astropy spans much larger length and time scales so it makes sense to be flexible with units.)

Yeah we can drop TRZ and nobody will likely notice.

The DLP stuff looks confusing, section 1.3.7 says those are the internal units, which could well be different to the input/output units...

Yeah we can drop TRZ and nobody will likely notice.

Do you mean dropping support for forces in TRZ (because only your own custom code was writing forces)? The IBisCO docs for the II. coordinate file format only mention positions and velocities

This file contains the coordinates, the velocities and the connectivity table.

so by dropping force support we would simply be implementing the published format. I would be ok with this approach and dropping non-standard features.

DL_Poly looks complicated. Cited from the manual

1.3.7 Units

[...]

Note: In the DL POLY 4 OUTPUT file, the print out of pressure is in units of katms (kilo-atmospheres) at all times. The unit of energy is either DL POLY units specified above, or in other units specified by the user at run time (see Section 7.1.3). The default is the DL POLY unit.
Externally, DL POLY 4 accepts information in its own specific formatting as described in Section 7.1. Irrespective of formatting rules, all values provided to define input entities are read in DL POLY units (except otherwise specified as in the case of energy units) or their composite mixture representing the corresponding entity physically, i.e. velocities’ components are in Ångstroms/picosecond.

It looks as if one can set units pretty freely in the input file (in particular the CONTROL and FIELD (force field) files).

However, it does not look as if the output files automatically carry the input file units. From the docs it looks as if the output files have well-defined, fixed units:

Here are some things I found

• 7.1.2.1 The CONFIG File Format (input coordinates): uses
  • Å for cartesian position coordinates (and cell box vectors)
  • Å/ps for velocities
  • Å * Dalton/ps² for forces

  Last but not least, it is worth pointing out that composite entities, such as velocities and forces, have their units expressed as composites of the default DL POLY units as shown in Section 1.3.7.

• REVCON (restart file) has the same format as the CONFIG file.
• 7.2.1 The HISTORY File (trajectory output)
  • time: ps
  • cell vectors: Å
  • atomic mass: amu (Dalton)
  • atomic charge: e
  • position: ? (assume Å – would be nice to state it even it it is "obvious")
  • velocity: Å/ps
  • force: Å * Dalton / ps²

I think we should start by assuming that files are always in the documented DL_POLY units and add a note to the reader/writer stating this explicitly.

So should DLPoly.ConfigReader and DLPoly.HistoryReader change to convert units when reading in forces? They don't seem to do that at present.

Yes, I think they should.

GSOC Contributor Inquiry

Hello!

I'm a prospective GSOC contributor hoping to work on this issue. I made the aforementioned edits to add unit conversions for force in the DL_Poly file with ConfigReader and HistoryReader and dropped the force-reading elements in TRZ in the highlighted locations. However, I have some questions before submitting a pull request.

1. I removed the error checks for forces in the _read_trz_header method, but I was wondering if it is necessary to remove force from the TRZ trajectory header as well, as I am unsure if this will disrupt later processing.

2. If I understood correctly, I need to add test cases to the respective files in the test suite. I was thinking of adding checks to see whether DL_Poly produces the right units and to see if the removal of force considerations in TRZ causes errors. I would love any recommendations on implementation or more tests that need to be written.

I am relatively new to open source, so any feedback is appreciated!

@lexi-x can you put up your PR (and mention this issue number in it so that it gets linked)? Looking at code — even incomplete one — is a lot easier than talking about it in the abstract. Once the tests run in GitHub actions, you'll also get a coverage report that shows which parts of your code changes need to be tested.

I also should say that this is a fairly old issue and things have been changing. For instance, we are going to drop TRZ support #4311 so we are probably not going to be very concerned with TRZ. For DL_Poly we'll have to look. If you can work with DL_Poly yourself then that would be really useful, I think, because it's not a program that I have any personal experience with.

@orbeckst Thank you for your response! I just created a draft pull request with what I have so far, I kept the TRZ edits in the pull request just in case but noted for the future that TRZ is not a priority. Meanwhile, I will keep exploring what I can do with DL_Poly. Please let me know any suggestions you have based on the pull request, thank you!