Lammps Dump Parser #3844

jaclark5 · 2022-09-21T16:01:46Z

Changes made in this Pull Request:

Allow box translation upon importing LAMMPS dump file
Allows coordinates to be unwrapped with dump file image flags
Allows import of velocities and forces from lammps dump file

PR Checklist

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

package/MDAnalysis/coordinates/LAMMPS.py

hmacdope

Thanks for the contribution @jaclark5! It's looking great so far. See my comments. We will also need some tests with a small imageflag containing testfile. :)

It may also be better to split the imageflag and forces+velocities stuff into separate PRs to avoid this one becoming super crowded but up to you at the end of the day.

package/MDAnalysis/coordinates/LAMMPS.py

hmacdope

Some more quick comments from a read over.

package/MDAnalysis/coordinates/LAMMPS.py

Co-authored-by: Hugo MacDermott-Opeskin <[email protected]>

codecov · 2022-09-28T01:02:25Z

Codecov Report

Base: 94.35% // Head: 94.35% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (db76d0f) compared to base (998a020).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3844   +/-   ##
========================================
  Coverage    94.35%   94.35%           
========================================
  Files          194      194           
  Lines        25031    25062   +31     
  Branches      3374     3390   +16     
========================================
+ Hits         23617    23648   +31     
  Misses        1365     1365           
  Partials        49       49

Impacted Files	Coverage Δ
package/MDAnalysis/coordinates/LAMMPS.py	`95.92% <100.00%> (+0.43%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

testsuite/MDAnalysisTests/datafiles.py

hmacdope

Few comments :)

package/CHANGELOG

package/MDAnalysis/coordinates/LAMMPS.py

hmacdope · 2022-10-01T03:22:44Z

package/MDAnalysis/coordinates/LAMMPS.py

+                    coords = coords[:3]
+                    coords += images * ts.dimensions[:3]
+                else:
+                    raise ValueError("Cannot unwrap coordinates without image flags.")


You will need to add a test that covers this.

hmacdope

Looking good, just a few suggestions.

package/CHANGELOG

package/MDAnalysis/coordinates/LAMMPS.py

hmacdope

Almost there, quick comment and will await some feedback from @PicoCentauri

package/CHANGELOG

package/MDAnalysis/coordinates/LAMMPS.py

hmacdope

I'm happy with this now. :) Anyone else want to review or shall we go ahead.

IAlibay

Couple things on my end, mostly pep8

testsuite/MDAnalysisTests/coordinates/test_lammps.py

package/MDAnalysis/coordinates/LAMMPS.py

IAlibay · 2022-10-24T10:24:49Z

package/MDAnalysis/coordinates/LAMMPS.py

@@ -455,7 +455,7 @@ class DumpReader(base.ReaderBase):
    """Reads the default `LAMMPS dump format`_

    Supports coordinates in the LAMMPS "unscaled" (x,y,z), "scaled" (xs,ys,zs),


We really need a Parameters section for this parser, this is too much text for folks to read and understand what's going on.

I'm happy having this be addressed in a follow-up PR, but can an issue be raised please?

testsuite/MDAnalysisTests/coordinates/test_lammps.py

IAlibay

lgtm thanks!

jaclark5 · 2022-10-26T13:00:01Z

Hmm I thought this was supposed to merge upon approval, do I need to do anything else?

IAlibay · 2022-10-26T13:01:22Z

Hmm I thought this was supposed to merge upon approval, do I need to do anything else?

I was just waiting on CI, I'll merge now (we don't have merge on approval set up, it's something the coredevs need to discuss at some point I guess)

jaclark5 added 3 commits September 21, 2022 11:36

Add check in LAMMPS Dump Reader for velocities and forces

ccf0a94

Translate LAMMPS coordinates to range from zero to box length

b4a4d97

Add unwrap feature to LAMMPSDump

098b907

github-actions bot added the Component-Readers label Sep 21, 2022

richardjgowers reviewed Sep 22, 2022

View reviewed changes

package/MDAnalysis/coordinates/LAMMPS.py Outdated Show resolved Hide resolved

hmacdope requested changes Sep 22, 2022

View reviewed changes

jaclark5 added 3 commits September 22, 2022 10:31

PR Response lammps dump reader

56bbe1d

PR Response lammps dump reader

a465d63

Merge branch 'lammps' of github.com:jaclark5/mdanalysis into lammps

0b7b461

hmacdope requested changes Sep 26, 2022

View reviewed changes

package/MDAnalysis/coordinates/LAMMPS.py Outdated Show resolved Hide resolved

package/MDAnalysis/coordinates/LAMMPS.py Outdated Show resolved Hide resolved

package/MDAnalysis/coordinates/LAMMPS.py Outdated Show resolved Hide resolved

Jennifer A Clark and others added 3 commits September 27, 2022 19:29

Update package/MDAnalysis/coordinates/LAMMPS.py

6d327d2

Co-authored-by: Hugo MacDermott-Opeskin <[email protected]>

Update Changelog and fix PR syntax for lammps dump

0b8f76b

Updated LAMMPSDUMP tests with box transformation

9e57281

Updates LAMMPSDUMP test for new features

f50b7fc

jbarnoud reviewed Sep 28, 2022

View reviewed changes

testsuite/MDAnalysisTests/datafiles.py Show resolved Hide resolved

jaclark5 added 2 commits September 28, 2022 07:27

Added LAMMPS test files reference in datafile.py

5c7e2f6

Moved LAMMPSDUMP vel and force check with ATOM card parsing

dae8480

jaclark5 marked this pull request as ready for review September 29, 2022 12:30

hmacdope requested changes Oct 1, 2022

View reviewed changes

jaclark5 and others added 2 commits October 1, 2022 12:29

PR formatting fixes

7a100fd

Merge branch 'develop' into lammps

278e049

jaclark5 requested a review from hmacdope October 11, 2022 21:15

hmacdope requested changes Oct 16, 2022

View reviewed changes

jaclark5 and others added 2 commits October 18, 2022 12:17

Incorporate reviewer comments

e56d98d

Merge branch 'develop' into lammps

2f8c905

jaclark5 requested a review from hmacdope October 20, 2022 21:18

hmacdope requested changes Oct 21, 2022

View reviewed changes

package/CHANGELOG Outdated Show resolved Hide resolved

package/MDAnalysis/coordinates/LAMMPS.py Show resolved Hide resolved

Make current with develop

cbaa894

orbeckst linked an issue Oct 21, 2022 that may be closed by this pull request

LAMMPS dump file unwrap trajectory and import velocities and forces #3843

Closed

hmacdope approved these changes Oct 22, 2022

View reviewed changes

IAlibay requested changes Oct 24, 2022

View reviewed changes

jaclark5 and others added 2 commits October 25, 2022 21:30

Modernized LAMMPS parser testing, documentation, and adherence to pep8

90e3e38

Merge branch 'develop' into lammps

db76d0f

IAlibay approved these changes Oct 26, 2022

View reviewed changes

IAlibay merged commit 5b2f447 into MDAnalysis:develop Oct 26, 2022

jaclark5 deleted the lammps branch November 17, 2022 14:05

orbeckst mentioned this pull request May 25, 2023

Automatic box translation in MDAnalysis 2.4 breaks re-centering operations during simulation #4148

Open

IAlibay added the enhancement label Sep 25, 2023

hejamu mentioned this pull request Sep 30, 2023

WIP: Lammpsdump velocities (and other fields) #3448

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lammps Dump Parser #3844

Lammps Dump Parser #3844

jaclark5 commented Sep 21, 2022 •

edited by orbeckst

Loading

hmacdope left a comment

hmacdope left a comment

codecov bot commented Sep 28, 2022 •

edited

Loading

hmacdope left a comment

hmacdope Oct 1, 2022

hmacdope left a comment

hmacdope left a comment

hmacdope left a comment

IAlibay left a comment

IAlibay Oct 24, 2022

IAlibay left a comment

jaclark5 commented Oct 26, 2022

IAlibay commented Oct 26, 2022

		@@ -455,7 +455,7 @@ class DumpReader(base.ReaderBase):
		"""Reads the default `LAMMPS dump format`_

		Supports coordinates in the LAMMPS "unscaled" (x,y,z), "scaled" (xs,ys,zs),

Lammps Dump Parser #3844

Lammps Dump Parser #3844

Conversation

jaclark5 commented Sep 21, 2022 • edited by orbeckst Loading

PR Checklist

hmacdope left a comment

Choose a reason for hiding this comment

hmacdope left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 28, 2022 • edited Loading

Codecov Report

hmacdope left a comment

Choose a reason for hiding this comment

hmacdope Oct 1, 2022

Choose a reason for hiding this comment

hmacdope left a comment

Choose a reason for hiding this comment

hmacdope left a comment

Choose a reason for hiding this comment

hmacdope left a comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay Oct 24, 2022

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

jaclark5 commented Oct 26, 2022

IAlibay commented Oct 26, 2022

jaclark5 commented Sep 21, 2022 •

edited by orbeckst

Loading

codecov bot commented Sep 28, 2022 •

edited

Loading