New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LAMMPSDUMP should check when returning corrdinates scaled with box dimensions #2135

Open
schlaicha opened this Issue Nov 7, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@schlaicha

schlaicha commented Nov 7, 2018

Expected behavior

Loading the testfile wat.lammpstrj should return the input coordinates.

Actual behavior

The coordinates returned by the reader are scaled with the box dimension.

Code to reproduce the behavior

import MDAnalysis as mda
u = mda.Universe('wat.lammpstrj', format='LAMMPSDUMP')
print (u.atoms.positions)

However,

print (u.atoms.positions/u.dimensions[:3])

returns the expexted coordinates.

Currently version of MDAnalysis

I tried 0.19.0 and 0.19.1. For some reason the same command with 0.19.2 returns an Error: need more than 2 values to unpack just after reading the first frame, but I guess that's a separate issue.

Additional Information

I realized in your corresponding test in reference_position you also multiply the coordinates by the box dimension?

So maybe I'm missing something in case this is done on purpose but in the source code I also did not see where the multiplication by the box length would happen...

@richardjgowers

This comment has been minimized.

Member

richardjgowers commented Nov 7, 2018

@schlaicha the rescaling of coordinates is by design, it's done in this line: https://github.com/MDAnalysis/mdanalysis/blob/develop/package/MDAnalysis/coordinates/LAMMPS.py#L573

I couldn't reproduce the error you mention, could you give more details on this?

@schlaicha

This comment has been minimized.

schlaicha commented Nov 7, 2018

Okay sorry I missed this line, I see your intention now.

The freedom of LAMMPS dumps makes some work to interpret the files: xs are scaled coordinates, but x are unscaled (so sorry for the trouble, your test is correct).

What I guess one needs to do is to evaluate the ITEM: ATOMS id type x(s) y(s) z(s) lines in order to interpret the coordinates correctly. Note that there is also xu and xus, https://lammps.sandia.gov/doc/dump.html.

Then this would be closely related to #2131. For me it would be okay to close this issue as duplicate and add a note that different coordinate scalings need to be implemented in #2131?

@richardjgowers

This comment has been minimized.

Member

richardjgowers commented Nov 7, 2018

Yeah it's no problem, maybe there should be an option to get the original coordinates, I just thought that the unscaled versions are the most useful to people?

So currently the dumpreader only works with the default format, and yeah #2131 looks like the place to get this working better.

@schlaicha

This comment has been minimized.

schlaicha commented Nov 7, 2018

Yeah it's no problem, maybe there should be an option to get the original coordinates, I just thought that the unscaled versions are the most useful to people?

But now you are assuming scaled coordinates, right? I have no opinion what is more useful, I guess it just needs to be clear why and what the dumpreader does (which now I think is not obvious for the user).

yeah #2131 looks like the place to get this working better.

I think the proper way is to evaluate the kind of coordinates and do the corresponding conversion directly, no need for another option (once #2131 is going to be implemented).

@schlaicha

This comment has been minimized.

schlaicha commented Nov 7, 2018

I couldn't reproduce the error you mention, could you give more details on this?

I just checked out the release tag again and could not reproduce it anymore, so just ignore this statement.

@richardjgowers richardjgowers changed the title from LAMMPSDUMP parser returns corrdinates scaled with box dimensions to LAMMPSDUMP should check when returning corrdinates scaled with box dimensions Nov 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment