Fix for issue #3383 triclinic lammpsdump #3403

alexgorfer · 2021-08-24T11:05:09Z

Changes made in this Pull Request:

DumpReader was fixed to expect triclinic boxes in the orthogonal bounding box format seen in e.g. https://docs.lammps.org/Howto_triclinic.html

PR Checklist

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

codecov · 2021-08-24T11:26:36Z

Codecov Report

Merging #3403 (5056fd5) into develop (e0a44cd) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #3403      +/-   ##
===========================================
+ Coverage    94.11%   94.15%   +0.03%     
===========================================
  Files          190      190              
  Lines        24654    24659       +5     
  Branches      3309     3309              
===========================================
+ Hits         23204    23217      +13     
+ Misses        1404     1396       -8     
  Partials        46       46

Impacted Files	Coverage Δ
package/MDAnalysis/coordinates/LAMMPS.py	`96.14% <100.00%> (+2.90%)`	⬆️
MDAnalysisTests/coordinates/base.py	`95.35% <0.00%> (+<0.01%)`	⬆️

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 e0a44cd...5056fd5. Read the comment docs.

hmacdope · 2021-08-31T23:05:34Z

Sorry @Agorfa, I am trying to triage and get to this ASAP.

alexgorfer · 2021-08-31T23:29:33Z

Hey @hmacdope no problem, thanks so much! With triaging you mean the failing checks or the issue this pr is concerned with? These checks for python builds 3.7+ all fail due to not having permission to read 'C:\Users\VssAdministrator\AppData\Local\Temp\pytest-of-VssAdministrator\pytest-0\popen-gw1\test_write_with_drivers_core_0h5md-writer-test.h5md' which should be something unrelated!?

For the actual issue I couldnt find versions older than 2019 of lammps to see if this has always been their way of dumping boxes.

IAlibay · 2021-08-31T23:31:22Z

@Agorfa for the H5MD failures, if you could update your branch against develop that should be fixed now.

hmacdope · 2021-08-31T23:32:06Z

Sorry @Agorfa, by triage I meant hopefully move up my priority list. :) Don't worry about the CI failures, they are unrelated as @IAlibay has just pointed out as I write this. :)

alexgorfer · 2021-08-31T23:35:49Z

Ah ok awesome :) Will do!

IAlibay

Just an initial quick review, it'd be really useful if you could add some tests that cover the use case being covered.

IAlibay · 2021-08-31T23:33:49Z

package/MDAnalysis/coordinates/LAMMPS.py

@@ -560,10 +560,15 @@ def _read_next_timestep(self):

        triclinic = len(f.readline().split()) == 9  # ITEM BOX BOUNDS
        if triclinic:
-            xlo, xhi, xy = map(float, f.readline().split())
-            ylo, yhi, xz = map(float, f.readline().split())
+            xlo_bound, xhi_bound, xy = map(float, f.readline().split())


Would it be possible to add a relevant test (maybe based on the issue this aims to fix) that covers these lines?

Ah of course, I can write a test that reads in a triclinic box. This would test that it doesnt fail. Testing that the actual issue would be resolved is difficult/impossible?. I think one would have to create a triclinic box in lammps, then actually run lammps and read the output to see if the dimensions are the same.

So here that's what we would require to do - create a triclinic box in lammps and then check that we reproduce expected outcomes (calculated by hand and/or comparing to a lammps output).

If we can't do such a correctness check then we might have to discuss further whether or not we can offer this option.

I have a first correctness check here:
triclinic_lammps_2021_09_02.zip
I use the values from the input data file for lammps that require xlo, xhi etc. directly and compare those dimensions to the ones imported from the lammpsdump file of the same system:

import MDAnalysis as mda import numpy as np from MDAnalysis.lib import mdamath u = mda.Universe("dump.atom", topology_format='LAMMPSDUMP') ## Direct values from lammps data file xlo = -0.32115478301032807 xhi = 16.831069399898624 ylo = -0.12372358703610897 yhi = 25.95896427399614 zlo = -0.045447071698045266 zhi = 12.993982724334792 xy = 1.506743915478767 xz = - 6.266414551929444 yz = - 0.42179319547892025 box = np.zeros((3, 3), dtype=np.float64) box[0] = xhi - xlo, 0.0, 0.0 box[1] = xy, yhi - ylo, 0.0 box[2] = xz, yz, zhi - zlo direct_dimensions = mdamath.triclinic_box(*box) print(direct_dimensions == u.dimensions)

resulting in the same dimensions

[ True True True True True True]

Would this be sufficient? Should I then write this procedure into a test file or what would be the best practice for testing specifications set in other software?

You can write something like this into the testsuite! the relevant file is here.

You will need to learn a bit of pytest, and use the text fixtures but hopefully the file is commented enough to be comprehensible. The numpy testing functions will be very helpful. See here for how to run the testsuite.

If you need to add an additional file to the test data, please do add it to the mdanalysis/testsuite/MDAnalysisTests/data/lammps folder and import it in
mdanalysis/testsuite/MDAnalysisTests/datafiles.py

I am going to tag @richardjgowers in case I am missing something about ortho vs triclinic in MDA/LAMMPS here?

Ah awesome thank you! I will get the keywords you recommended and then write it up in there!

hmacdope · 2021-09-02T02:14:35Z

Note we should also improve checking for keywords in the ITEM BOX BOUND check, just checking for triclinic = len(f.readline().split()) == 9 is not super safe.

hmacdope · 2021-09-02T03:05:19Z

Sorry for the spam, but it might be good to use the thermo_style custom cella, cellb, cellc, cellalpha, cellbeta, cellgamma, keyword for to generate test data that we can compare our triclinic representation to directly.

alexgorfer · 2021-09-07T04:42:31Z

Sorry for the spam, but it might be good to use the thermo_style custom cella, cellb, cellc, cellalpha, cellbeta, cellgamma, keyword for to generate test data that we can compare our triclinic representation to directly.

Ah thats a good idea! I will do a short simulation with these keywords

…cell

pep8speaks · 2021-09-07T04:57:00Z

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

In the file testsuite/MDAnalysisTests/datafiles.py:

Line 139:28: E261 at least two spaces before inline comment
Line 139:80: E501 line too long (113 > 79 characters)
Line 144:28: E261 at least two spaces before inline comment
Line 144:80: E501 line too long (113 > 79 characters)
Line 495:80: E501 line too long (87 > 79 characters)
Line 500:80: E501 line too long (87 > 79 characters)

Comment last updated at 2022-03-23 01:39:02 UTC

hmacdope · 2021-09-07T12:33:45Z

Just ping me when you want a review @Agorfa 👍

hmacdope · 2021-11-01T11:31:19Z

Hey @Agorfa, any progress on this? No rush, but I think it would be good to fix this up so that LAMMPs users are getting the right triclinic boxes. :) I'm happy to contribute code and/or discuss if anything is unclear.

alexgorfer · 2021-11-02T01:19:08Z

Hey @hmacdope! Sorry, it has really been on my mind but I didnt do all that much yet.. Yes, I think it would be really good to finish this. I will try to make good progress this week!

hmacdope · 2021-11-02T02:42:45Z

No rush :) Only when you have time 👍

alexgorfer · 2021-12-19T19:53:36Z

Hey @hmacdope, at long last, I finally have something new to show. In my test I compare the boxes between a .dump file to the .data file to my expectation, all for the same system. Is this ok?

hmacdope · 2021-12-21T00:06:10Z

Hi @Agorfa thank you! This seems like the right approach to me. :) I will leave a detailed review soon.

hmacdope

Hi @agorfa looking really good! Just a few nitpicks here and there.

testsuite/MDAnalysisTests/datafiles.py

testsuite/MDAnalysisTests/coordinates/test_lammps.py

testsuite/MDAnalysisTests/data/lammps/albite_triclinic.dump

testsuite/MDAnalysisTests/coordinates/test_lammps.py

package/MDAnalysis/coordinates/LAMMPS.py

alexgorfer · 2021-12-21T23:13:55Z

Thanks so much for the review! I will implement it as soon as I can!

hmacdope · 2022-03-08T22:32:20Z

@Agorfa, we would love to get this fixed soon if you are interested in continuing this PR. :)

alexgorfer · 2022-03-08T23:00:48Z

Hi @hmacdope ! Yes sry of course I will implement it asap!

hmacdope · 2022-03-08T23:08:28Z

@Agorfa no need to apologise! We are super grateful for the contribution. :)

alexgorfer · 2022-03-09T01:41:06Z

Hey I implemented your conversations! I dont understand why codecov/project complains :S

hmacdope · 2022-03-09T02:15:49Z

@orbeckst I would love a second set of eyes on this box math

orbeckst

I only reviewed the triclinic math. This all looks good to me so I approve. Thank you for your contribution!

Please update the docs for the LAMMPS reader to note that triclinic boxes are read and link to the LAMMPS documentation.

I am leaving it to @hmacdope and @IAlibay to also make sure that you also add yourself to AUTHORS and include a CHANGELOG entry.

package/MDAnalysis/coordinates/LAMMPS.py

hmacdope · 2022-03-09T22:30:31Z

Thanks @orbeckst!

@Agorfa, would you be able to update the docs as suggested and add yourself to AUTHORS and an entry to CHANGELOG please? Then we will be ready to merge. 🥇

IAlibay · 2022-03-09T22:46:06Z

@Agorfa we're trying to get a means of contacting folks in the future as we consider changing the MDAnalysis license. Could you please send an email to the MDAnalysis developer list introducing yourself (with your GitHub handle) before we merge this PR?

alexgorfer · 2022-03-10T04:06:18Z

Hey everyone! Thanks so much for your guidance! I wrote docs, changelogs and authors. I'm not sure about the .. versionchanged:: 2.2.0 I added I guessed it is helpful for some users? I hope everything else is at the right place

alexgorfer · 2022-03-10T04:21:02Z

Hey @IAlibay of course I just sent it!

hmacdope

This all looks good to me with CHANGELOG and CONTRIBUTORS updated along with adequate docs Thanks so much for your contribution @Agorfa. Have I missed anything @IAlibay?

hmacdope · 2022-03-20T22:46:13Z

@IAlibay have your proposed changes been addressed?

@Agorfa If you could resolve conflicts that would be amazing so I can merge pending @IAlibay.

alexgorfer · 2022-03-20T23:15:10Z

Should be resolved!

IAlibay

Just a couple of small things, please do address. I'll give this an approval so my review isn't blocking.

IAlibay · 2022-03-21T15:44:02Z

package/MDAnalysis/coordinates/LAMMPS.py

+    length unit (Å), and angles are in degrees.
+
+    .. versionchanged:: 2.2.0
+       Triclinic simulation boxes are supported. (Issue #3383)


It might be good to make that issue number a link to the actual issue.

package/CHANGELOG

package/MDAnalysis/coordinates/LAMMPS.py

alexgorfer · 2022-03-23T01:51:41Z

Ah sorry thanks I got confused with the order when I resolved the conflict.. I also made the link and pep8 seems happy

hmacdope · 2022-03-23T04:29:05Z

Congratulations @Agorfa! We appreciate your contribution.

orbeckst · 2022-03-23T15:45:49Z

Thank you for your contribution @Agorfa , well done! I hope you feel encouraged to contribute more in the future!

hmacdope added Component-Readers format-LAMMPS labels Aug 24, 2021

IAlibay requested changes Aug 31, 2021

View reviewed changes

changes DumpReader to expect orthogonoal bounding boxes of triclinic …

5c10286

…cell

github-actions bot added Component-Core Continuous Integration labels Sep 7, 2021

alexgorfer force-pushed the origin/issue-3383 branch from 7166179 to 5c10286 Compare September 7, 2021 05:51

github-actions bot removed Continuous Integration Component-Core labels Sep 7, 2021

alexgorfer added 2 commits December 19, 2021 18:39

test between data, dump and expectation

e7c5af4

pep8 fixes

ad456c7

hmacdope requested changes Dec 21, 2021

View reviewed changes

alexgorfer added 2 commits March 9, 2022 00:52

comments and assert_allcose

ef2ef22

Merge branch 'MDAnalysis:develop' into origin/issue-3383

c2c331c

alexgorfer requested a review from hmacdope March 9, 2022 01:45

hmacdope requested a review from orbeckst March 9, 2022 02:15

orbeckst approved these changes Mar 9, 2022

View reviewed changes

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

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

doc and changelog

3dd0cdd

hmacdope approved these changes Mar 15, 2022

View reviewed changes

Merge branch 'develop' into origin/issue-3383

5b14120

IAlibay approved these changes Mar 21, 2022

View reviewed changes

alexgorfer added 2 commits March 23, 2022 02:30

Changelog, IssueLink, pep8

deb9e64

pep8

5056fd5

hmacdope merged commit 7670dad into MDAnalysis:develop Mar 23, 2022

IAlibay added the defect label Sep 25, 2023

Fix for issue #3383 triclinic lammpsdump #3403

Fix for issue #3383 triclinic lammpsdump #3403

Conversation

alexgorfer commented Aug 24, 2021 • edited Loading

PR Checklist

codecov bot commented Aug 24, 2021 • edited Loading

Codecov Report

hmacdope commented Aug 31, 2021

alexgorfer commented Aug 31, 2021

IAlibay commented Aug 31, 2021

hmacdope commented Aug 31, 2021

alexgorfer commented Aug 31, 2021

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay Aug 31, 2021

Choose a reason for hiding this comment

alexgorfer Aug 31, 2021

Choose a reason for hiding this comment

IAlibay Sep 1, 2021

Choose a reason for hiding this comment

alexgorfer Sep 2, 2021

Choose a reason for hiding this comment

hmacdope Sep 2, 2021

Choose a reason for hiding this comment

alexgorfer Sep 7, 2021

Choose a reason for hiding this comment

hmacdope commented Sep 2, 2021

hmacdope commented Sep 2, 2021

alexgorfer commented Sep 7, 2021

pep8speaks commented Sep 7, 2021 • edited Loading

Comment last updated at 2022-03-23 01:39:02 UTC

hmacdope commented Sep 7, 2021

hmacdope commented Nov 1, 2021

alexgorfer commented Nov 2, 2021

hmacdope commented Nov 2, 2021

alexgorfer commented Dec 19, 2021

hmacdope commented Dec 21, 2021

hmacdope left a comment

Choose a reason for hiding this comment

alexgorfer commented Dec 21, 2021

hmacdope commented Mar 8, 2022

alexgorfer commented Mar 8, 2022

hmacdope commented Mar 8, 2022

alexgorfer commented Mar 9, 2022

hmacdope commented Mar 9, 2022

orbeckst left a comment

Choose a reason for hiding this comment

hmacdope commented Mar 9, 2022

IAlibay commented Mar 9, 2022

alexgorfer commented Mar 10, 2022

alexgorfer commented Mar 10, 2022

hmacdope left a comment • edited Loading

Choose a reason for hiding this comment

hmacdope commented Mar 20, 2022

alexgorfer commented Mar 20, 2022

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay Mar 21, 2022

Choose a reason for hiding this comment

alexgorfer commented Mar 23, 2022

hmacdope commented Mar 23, 2022

orbeckst commented Mar 23, 2022

alexgorfer commented Aug 24, 2021 •

edited

Loading

codecov bot commented Aug 24, 2021 •

edited

Loading

pep8speaks commented Sep 7, 2021 •

edited

Loading

hmacdope left a comment •

edited

Loading