search

MDAnalysis / mdanalysis

MDAnalysis is a Python library to analyze molecular dynamics simulations.
https://mdanalysis.org
Other
1.32k stars 652 forks source link

Document possible -L/2 -> +L/2 box convention effect on use of wrap when reading coordinates #3363

Open hmacdope opened 3 years ago

hmacdope commented 3 years ago

Is your feature request related to a problem?

We should gracefully handle or and possibly warn users when the -L/2 -> +L/2 box convention is used rather than 0 -> L box convention, as this may possibly lead to unexpected downstream behaviour. See discussion on #3360, particularly comments by @p-j-smith as LAMMPS is one such package that happily uses negative box bounds.

Additional context

Can other packages use negative box bounds? Is this something we would need to standardise or can we address one Reader at a time.

orbeckst commented 3 years ago

Is the main problem that wrap/unwrap use the [0, L] box instead of [-L/2, L/2], i.e., is the issue that all our PBC processing code assumes [0, L]?

orbeckst commented 3 years ago

@p-j-smith please feel free to weigh in here, especially as you started this issue with your https://github.com/MDAnalysis/mdanalysis/pull/3360#issuecomment-879866369.

hmacdope commented 3 years ago

I interpreted it to mean that -L/2->L/2 is may not be always correctly treated, but again this was interpretation of @richardjgowers and @p-j-smith's comments, rather than my own knowledge of the pbc code.

p-j-smith commented 3 years ago

Sorry for the confusion. I don't think there is anything incorrect about the treatment of PBC per se. I think it's fine to use [0, L] instead of [-L/2, L/2] for PBC processing. However, LAMMPS users that are new to MDAnalysis might be surprised to find that u.dimensions doesn't contain [[xlo, xhi], [ylo, yhi], [zlo, zhi]] (the lower and upper bounds can be any value, they need not be [-L/2, L/2]). They would probably also expect u.atoms.wrap() to wrap atoms between the bounds [[xlo, xhi], [ylo, yhi], [zlo, zhi]] rather than [[0, Lx], [0, Ly], [0, Lz]].

I'm not suggesting u.dimensions or u.atoms.wrap() should be changed to accommodate these expectations, but it would be nice for LAMMPS users if the behaviour were documented, perhaps in the User Guide for reading LAMMPS data files or as a warning when creating a universe from a LAMMPS data file.

hmacdope commented 3 years ago

Roger roger, mostly my overintepretation at fault here. Shall we channel this perhaps towards documentation.

EDIT: I will change the issue name to better reflect this.

IAlibay commented 3 years ago

Beyond just documenting, could we get a couple tests added here to check that behaviour actually is correct? I remember there being some discussion by @mnmelo about the assumption of how boxes work in MDA. If anything, even if it works now, having a test for this will make sure we don't break this in the future.