WIP: Lammpsdump velocities (and other fields) #3448
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 |
There was a problem hiding this comment.
Hi @schlaicha thanks for starting this. Yes this looks like the correct approach to get this working.
| @@ -502,7 +504,7 @@ def _reopen(self): | |||
| 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
| @@ -606,15 +608,32 @@ def _read_next_timestep(self): | |||
|
|
|||
| coord_cols = convention_to_col_ix[self.lammps_coordinate_convention] | |||
|
|
|||
| # 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. |
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