Skip to content

Fix reading of lammpsdump ATOM card. #3360

Merged
IAlibay merged 67 commits intoMDAnalysis:developfrom
hmacdope:fix-lammpsdump-parser
Aug 21, 2021
Merged

Fix reading of lammpsdump ATOM card. #3360
IAlibay merged 67 commits intoMDAnalysis:developfrom
hmacdope:fix-lammpsdump-parser

Conversation

@hmacdope
Copy link
Member

@hmacdope hmacdope commented Jul 6, 2021

Fixes #3358
Supersedes #2333
Changes made in this Pull Request:

  • Allow coordinates in different conventions to be read by LAMMPSDumpReader
  • Unhardcode LAMMPSDumpReader to expect a set number of columns
  • S->R transform with distances.transform_StoR is no longer applied by default, only if the coordinates are in the scaled (xs,ys,zs) or scaled_unwrapped (xsu, ysu, zsu) convention
  • On the way to allowing reading of other ts data from the LAMMPS dump file.

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@pep8speaks
Copy link

pep8speaks commented Jul 6, 2021

Hello @hmacdope! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 459:80: E501 line too long (109 > 79 characters)
Line 591:80: E501 line too long (89 > 79 characters)
Line 601:80: E501 line too long (81 > 79 characters)
Line 604:14: E713 test for membership should be 'not in'
Line 605:80: E501 line too long (122 > 79 characters)

Line 140:80: E501 line too long (100 > 79 characters)
Line 141:80: E501 line too long (125 > 79 characters)
Line 490:80: E501 line too long (83 > 79 characters)
Line 491:80: E501 line too long (95 > 79 characters)
Line 492:80: E501 line too long (93 > 79 characters)

Line 275:1: E302 expected 2 blank lines, found 1

Comment last updated at 2021-08-21 05:17:47 UTC

@codecov
Copy link

codecov bot commented Jul 6, 2021

Codecov Report

Merging #3360 (11d8518) into develop (bd0ed5d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #3360   +/-   ##
========================================
  Coverage    93.70%   93.70%           
========================================
  Files          177      177           
  Lines        22990    23017   +27     
  Branches      3247     3257   +10     
========================================
+ Hits         21542    21569   +27     
  Misses        1397     1397           
  Partials        51       51           
Impacted Files Coverage Δ
package/MDAnalysis/coordinates/LAMMPS.py 93.23% <100.00%> (+0.66%) ⬆️
package/MDAnalysis/topology/LAMMPSParser.py 96.23% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd0ed5d...11d8518. Read the comment docs.

Copy link
Member

@richardjgowers richardjgowers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when multiple coordinate columns are present?

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of quick comments, probably needs more tests?

hmacdope and others added 2 commits July 6, 2021 21:10
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
@hmacdope
Copy link
Member Author

I think I have addressed everyone's comments :)

@hmacdope
Copy link
Member Author

Do people have any further comments? :)

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're nearly there, but I'm a little bit hesitant since I'm not sure how modular the standard is, can coordinates be in the wrong order? Or does it not matter - does the way we loop through coord_cols on setting positions enforce ordering?

@MDAnalysis/coredevs if you have a chance to look here, maybe we can sneak this into 2.0.

@IAlibay IAlibay mentioned this pull request Aug 21, 2021
2 tasks
hmacdope and others added 6 commits August 21, 2021 13:52
Co-authored-by: Lily Wang <31115101+lilyminium@users.noreply.github.com>
Co-authored-by: Lily Wang <31115101+lilyminium@users.noreply.github.com>
@hmacdope hmacdope requested review from IAlibay and lilyminium August 21, 2021 05:15
Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@IAlibay
Copy link
Member

IAlibay commented Aug 21, 2021

@richardjgowers do you want to review / approve since you still have a blocking review?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LAMMPS DumpReader returns incorrect triclinic box dimensions LAMMPS DumpReader does not return the correct coordinates for unwrapped trajectories.

7 participants