Skip to content
This repository has been archived by the owner on Jan 31, 2024. It is now read-only.

Removing units from <file> tag #172

Closed
grhawk opened this issue Mar 13, 2017 · 7 comments
Closed

Removing units from <file> tag #172

grhawk opened this issue Mar 13, 2017 · 7 comments

Comments

@grhawk
Copy link
Contributor

grhawk commented Mar 13, 2017

Following #162, the idea is to remove units from the file tag to avoid confusion. The units specification can only be done into the xyz or pdb files.

@grhawk grhawk added this to the release-2.0 milestone Mar 13, 2017
@OndrejMarsalek
Copy link
Collaborator

I would like to respectfully but strongly oppose the use of mutated versions of standard formats like PDB. With the recent (or "recent", I should have got involved earlier, sorry) changes, we are in a situation where a correct to-specification PDB file will be interpreted incorrectly by i-PI, to confusing and potentially harmful results. Units are strictly a part of the format. What is needed instead is a file in a new custom format that is derived from PDB, is not PDB, but is still called PDB. I think that is very dangerous and should be avoided at all costs.

For XYZ, the situation is more complicated because there isn't a formal specification, so both units and box information unfortunately have some freedom. But even for that, there is for example the extended XYZ format used by several programs, see for example here or here.

I know that historically this has been treated relatively freely in i-PI, but I think especially if one wants to have users who are not directly related to the development of the program, respecting standard formats is important. I would consider using existing code from elsewhere or even whole libraries (perhaps optionally) and would be very happy to help us get there.

@ceriottm
Copy link
Collaborator

ceriottm commented Apr 17, 2017 via email

@OndrejMarsalek
Copy link
Collaborator

As far as I can tell, currently, a vanilla PDB will be read so that the numbers in it are interpreted as Bohr.

I agree that I/O changes have been painful, but I think that is still preferable to a painful experience every time a user tries to bring in non i-PI files. I suggest that we go over what the outstanding issues are and address as many of the smaller non-breaking fixes as possible before doing anything major. @grhawk collect many (most?, all?) of outstanding I/O-related issues in #151.

@grhawk
Copy link
Contributor Author

grhawk commented Apr 18, 2017

@OndrejMarsalek A vanilla pseudo-pdb (only compatible fields) should be treated in the right way. If it is treated assuming bohr as units, that is a bug. In that case, could you send me input and output of your example?

About the #151, I think it contains, at least, the major things that should be done. It is pending, mostly because the idea was doing a major revision of the IO and just fixing thinks in a dirty way could be a waste of time while rewriting the IO stuff is anyway really time demanding.

@OndrejMarsalek
Copy link
Collaborator

Reported the issue in #179.

Let's center all the effort to clean it up, eventually, around #151, then.

@ceriottm
Copy link
Collaborator

Hi @OndrejMarsalek I think this is connected with the final fix of the units conundrum.
I think in the new branch things are quite clear(er) - you can specify units in a PDB file, and/or in the xml field, but the default - specific to PDB - is that we are reading angstrom. Also, a warning (or error) is raised if one tries to use PDB for anything but the positions. happy with this?

@venkatkapil24
Copy link
Collaborator

I think this can be closed since we decided to keep the units in the file tag and the input file. If they are incompatible an error is raised.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants