Fix units (for good)

[ceriottm]

Let's face it. The latest round of IO/units changes has been a clusterf**k. @ipoltavskyi found it was slowing down things too much, @mahrossi and @venkatkapil24 reported that the units specification in the input was confusing and leading to more problems than it fixed. So here is another try at getting this right. I hope y'all can contribute to making sure we don't have to touch this boring part of the code in the next year at least. Main concepts are:

• one should be able to specify units both in the input and in the structure file. if no units are specified in the input, i-PI tries to guess from the comment line, otherwise assumes atomic units, or angstrom for PDB.
• if units are specified in both places, and match, conversion happens just once. if they mismatch, an error is raised.
• conversion happens in the IO functions as far as possible. this might also be a prequel to a more flexible output infrastructure, but for the moment main change is trajectory outputting passes around indication of units, but the conversion happens in print_file
• read_file_raw (and possibly print_file_raw?) are added to access the positions and labels with no conversion and no creation of i-pi objects

[mahrossi]

👍

[mahrossi]

I suggest that this infrastructure gets a proper documentation in the documentation if possible before the branch is merged? That way we don't forget to document it.

[mahrossi]

Hello: what I read and what I tested works as intended. I'm asking for the documentation anyways. I think that one big issue with all the previous infrastructures is that nothing was properly documented. In fact we need to look carefully at the documentation, but we can start small with this one. How to best do it?

[ceriottm]

Excellent suggestion, can you take care of that? I think currently the discussion of units is scattered in different places. Perhaps it might be nice to have a paragraph in the manually-written part of the docs that covers the philosophy behind units in i-PI

…

On 20 June 2017 at 15:56, Mariana Rossi ***@***.***> wrote: Hello: what I read and what I tested works as intended. I'm asking for the documentation anyways. I think that one big issue with all the previous infrastructures is that nothing was properly documented. In fact we need to look carefully at the documentation, but we can start small with this one. How to best do it? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#188 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABESZ1mc1ZRwjw491TVV2hLvEfYg1TWwks5sF8-lgaJpZM4N9zil> .

[mahrossi]

Yes, this was my next question: So we just put it in the manually written part of the manual? If so I'll just do it in this branch.

[ceriottm]

@OndrejMarsalek and @ipoltavskyi I'd like to hear your opinion, and -- possibly -- to have your help making sure that all post-processing tools still work with this revised infrastructure

[ipoltavskyi]

Dear Michele,

My scripts should be modified for the new infrastructure. I will do it ASAP. Also I would add some warning when PDB format is used for output. The actual coordinates may not match the allowed digits in this format, which will lead to a mess in the trajectory files. An example is qtip water in the examples folder. If you run this calculations you will find that the water molecules go away from the original box and may have rather large coordinates. PDB output is useless in this case.

Best wishes
Igor

[ceriottm]

Hi all, @ipoltavskyi in particular. Any chance we can move this forward, fixing tools and merging ASAP?

[ipoltavskyi]

Hi guys. To be honest I do not understand this new infrastructure with automatic units and increased number of functions for no obvious reasons (for instance "print_file_raw" function is just a copy of the "return" line in "print_file" function with extra parameters which is used only once in the whole code). In fact the half of "auto_units" function in "io_units.py" contain "if" conditions in order to change useless "automatic" unit into "atoms_unit" or something else. I thought that the idea of this brunch was to make the code lighter and faster. Now it looks heavier and less user friendly. I do not get the idea behind these changes.

[ceriottm]

Hello Igor, the idea was twofold:
(1) solve the "units specified in the xml or in the file" conundrum, having units specified in the xml take precedence, and avoiding double conversions
(2) provide for each I/O funcion a "raw" mode that assumes data is already passed (or gathered) in the desired units, and a normal function that also takes care of units conversion
basically the idea would be that in your script you can always use "raw" functions and hardcode unit conversions so that "ifs" don't get evaluated at each frame

[ipoltavskyi]

OK, I see. But right now the implementation simply does not work at all. I think it will be better if the person who had made all this changes first make the branch comparable with the master branch. For instance, I cannot run now even the harmonic oscillator example. Errors with files writing functions. Then, we can think how efficient the code is and do some tests. Now the brunch is in a semi finished state. And I still do not understand why we need "automatic" units. The default can be "atomic_unit", and then we just check the comment line for units specification. This removes a lot of if statements which must be performed every step right now. If someone need raw data, it is much easier to add a key and if the key is true just skip all units transformation. But, the first is first. Right now the branch is simply broken.

[ceriottm]

merged with master. tried a couple of examples and they seem to work. "automatic" is there to handle the heuristics to decide where the units should be taken from. if one does not specify units "automatic" is passed, which is different then if you specify "atomic_units" as then your request should override units inferred from the input file. the whole point is that now, if you really care about those fractions of a ms, you can skip all these checks and call the _raw functions that give you access without any of these checks performed.

…

On 18 September 2017 at 16:12, ipoltavskyi ***@***.***> wrote: OK, I see. But right now the implementation simply does not work at all. I think it will be better if the person who had made all this changes first make the branch comparable with the master branch. For instance, I cannot run now even the harmonic oscillator example. Errors with files writing functions. Then, we can think how efficient the code is and do some tests. Now the brunch is in a semi finished state. And I still do not understand why we need "automatic" units. The default can be "atomic_unit", and then we just check the comment line for units specification. This removes a lot of if statements which must be performed every step right now. If someone need raw data, it is much easier to add a key and if the key is true just skip all units transformation. But, the first is first. Right now the branch is simply broken. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#188 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABESZ8zci6gIv9H3Jou3CB9UD1zBucx-ks5sjno1gaJpZM4N9zil> .

[ceriottm]

BTW, @ipoltavskyi please read the description of the pull request. It's months old but explains quite clearly what is the rationale and what we're trying to achieve. If you disagree with some of the objectives, I'm open to discussion, but reading that part carefully might help understand why there are new functions, and why the units conversion has moved to io from the trajectory outputs.py code

[ceriottm]

@venkatkapil24 there is still a request for you to clean up also the iter_file functions. Also, could you help making sure all postprocessing tools are compatible with the new infrastructure (perhaps wait until we are all on the same page with architecture before doing that). Thanks!

[ceriottm]

Hey all, @ipoltavskyi and @OndrejMarsalek in particular. Venkat has implemented iterator routines, and cleaned up a bit the code. I am all for open development and taking into account everyone's opinion but this PR has been open and tagged for review for three months. Please have a look, benchmark if you care about speed, and help making sure all the code paths and examples are compatible and working. If I don't hear anything, I'll merge early next week, and then avoid touching units until i-PI v3.0

[venkatkapil24]

just doing some last minute checks before pushing.

…

On Thu, Sep 21, 2017 at 2:33 PM, Michele Ceriotti ***@***.***> wrote: Hey all, @ipoltavskyi <https://github.com/ipoltavskyi> and @OndrejMarsalek <https://github.com/ondrejmarsalek> in particular. Venkat has implemented iterator routines, and cleaned up a bit the code. I am all for open development and taking into account everyone's opinion but this PR has been open and tagged for review for three months. Please have a look, benchmark if you care about speed, and help making sure all the code paths and examples are compatible and working. If I don't hear anything, I'll merge early next week, and then avoid touching units until i-PI v3.0 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#188 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKQjG3q7U6P_E2JRC34uZnqM3fHOUwUKks5skleKgaJpZM4N9zil> .

-- Venkat Kapil Doctoral Assistant - Ph.D Student EPFL STI IMX COSMO LABORATORY OF COMPUTATIONAL SCIENCE AND MODELLING Institute of Materials Science and Engineering MXG 339 Station 12 CH-1015 Lausanne Switzerland E-mail: venkat.kapil@gmail.com http://cosmo.epfl.ch/

[venkatkapil24]

read_file is now silenced (as in the info statement is commented) since it leads to an absurd number of printouts during post processing.
Can the creator of xyz2bin and bin2xyz check if the results are exactly as expected ? @ceriottm : Can you test if kinetic2tag is working (just for the sake of completeness).
Also, I have not touched the PPI related tools.