WIP: Lammpsdump velocities (and other fields)#3448
WIP: Lammpsdump velocities (and other fields)#3448schlaicha wants to merge 5 commits intoMDAnalysis:developfrom
Conversation
|
Hello @schlaicha! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2022-04-02 00:30:10 UTC |
richardjgowers
left a comment
richardjgowers
left a comment
There was a problem hiding this comment.
Hi @schlaicha thanks for starting this. Yes this looks like the correct approach to get this working.
| self.close() | ||
| self._file = util.anyopen(self.filename) | ||
| self.ts = self._Timestep(self.n_atoms, **self._ts_kwargs) | ||
| self.ts.frame = -1 |
There was a problem hiding this comment.
this is deliberately -1 as it gets incremented in the loading process
hmacdope
left a comment
hmacdope
left a comment
There was a problem hiding this comment.
Thanks so much for working on this! A few comments, but generally looking good. 👍
|
|
||
| # pass possible additional fields from LAMMPS dump | ||
| # pass velocities | ||
| velocity_col_names = ["vx", "vy", "vz"] |
There was a problem hiding this comment.
I would make this a class attribute, much like self._conventions is, as the data is not mutable or instance dependent. :)
| for cv_name, cv_col_names in enumerate(velocity_col_names): | ||
| try: | ||
| velocity_cols = [attr_to_col_ix[x] for x in velocity_col_names] | ||
| ts.has_velocities = True |
There was a problem hiding this comment.
This works, but I think we only want to set ts.has_velocities = True on exit from this loop, slightly cleaner IMO.
| if ts.has_velocities: | ||
| ts.velocities = ts.velocities[order] | ||
| # LAMMPS reader only supports real units | ||
| ts.velocities *= units.speedUnit_factor['Angstrom/femtosecond'] |
There was a problem hiding this comment.
Query @richardjgowers on units? I'm no lammps expert, what are the range of possible units?
|
I am going to close and re-open to trigger CI. |
|
@schlaicha are you still interested in completing the PR? |
Sorry for the long delay, in fact I forgot about that one. My collaborator @pstaerk started implementing this in a more general way, I believe, and thus I would suggest that we try to merge this with #3608 We will come back to this after having a first comment on #3608 |
|
@schlaicha the changes in this PR have been superseded by #3844. |
|
Thanks @hejamu for reminding me about this open PR. Closing as it is outdated. |
6 participants
Following #3360 (thanks for the great work!) I was implementing a first draft of a parser for the velocities.
Before adding tests I quickly wanted to hear if the way of implementing agrees with your concepts.
Other fields, like e.g. forces, could be added straightforward.
Fixes #
Changes made in this Pull Request:
PR Checklist