Add 'PairIJ Coeffs' section to the list of sections in LAMMPS parser #3959

mglagolev · 2022-12-15T21:57:23Z

When called with "pair ij" option, the write_data command in LAMMPS writes a data file with a 'PairIJ Coeffs' section, as stated https://docs.lammps.org/write_data.html
Absence of this section in the SECTIONS list of LAMMPSParser.py can lead to an error.
This fix ensures correct processing of LAMMPS data files with a 'PairIJ Coeffs' section.

Changes made in this Pull Request:

'PairIJ Coeffs' added to the list of sections in LAMMPSParser.py

… This ensures correct processing of LAMMPS data files

github-actions

Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on the developer mailing list so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.

codecov · 2022-12-15T22:13:47Z

Codecov Report

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

Coverage data is based on head (d8fc45a) compared to base (d67b4f1).
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3959   +/-   ##
========================================
  Coverage    93.49%   93.49%           
========================================
  Files          190      190           
  Lines        24963    24963           
  Branches      3527     3527           
========================================
+ Hits         23338    23340    +2     
+ Misses        1102     1101    -1     
+ Partials       523      522    -1

Impacted Files	Coverage Δ
package/MDAnalysis/topology/LAMMPSParser.py	`95.81% <ø> (+0.83%)`	⬆️
package/MDAnalysis/visualization/streamlines_3D.py	`70.49% <0.00%> (ø)`

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.

mglagolev · 2022-12-15T22:35:04Z

This should actually solve the issue #3336

hmacdope · 2022-12-15T23:11:30Z

Hi @mglagolev thanks for the contribution!

Would you be able to add a test with a small example test file to show the issue is resolved.

You will also need to update CHANGELOG, and add yourself to AUTHORS.

I would also ask you to possibly introduce yourself on the developer mailing list to welcome you to the community and have a means of contacting you. :)

mglagolev · 2022-12-16T09:49:16Z

@hmacdope Thank you for the suggestions! I pushed the changes to AUTHORS and CHANGELOG to my branch, shall I do anything else? This is my first pull request ever.
Shall I add the test file and test script to my branch also? If so, where shall I put it?

mglagolev · 2022-12-16T17:24:51Z

Added a datafile and a reading script to testsuite/MDAnalysisTests/topology

hmacdope

A good start :) See my comments. Ping me if unsure of anything.

hmacdope · 2022-12-19T05:12:08Z

testsuite/MDAnalysisTests/topology/PR3959_test.py

@@ -0,0 +1,17 @@
+#!/usr/bin/env python3


This can become part of the other LAMMPS Topology tests in test_lammpsdata.py it doesn't need its own file.

hmacdope · 2022-12-19T05:12:54Z

testsuite/MDAnalysisTests/topology/PR3959_test.py

+import MDAnalysis as mda
+
+
+PAIRIJ_COEFFS_DATA = "PR3959_test.data"


This needs to be added as a datafile see datafiles.py on how to do that.

hmacdope · 2022-12-19T05:14:26Z

testsuite/MDAnalysisTests/topology/PR3959_test.data

@@ -0,0 +1,3228 @@
+LAMMPS data file via write_data, version 23 Jun 2022, timestep = 1000


This needs to be moved to data and added to datafiles.py.

This reverts commit 383423f. The test needs to be integrated into the test_lammpsdata.py instead of a separate script, and the sample datafile needs to be placed in the appropriate directory.

mglagolev · 2022-12-22T15:12:51Z

@hmacdope Thank you for your guiding! I tried to replicate what I saw in test_lammpsdata.py, and committed to the branch. When I run the test:

cd testsuite/MDAnalysisTests
pytest --disable-pytest-warnings

It shows errors in some other modules, but not topology.
However, I'm not sure that I've done everything correctly and the testing actually takes place.

I'm also wondering, shall my datafile state the number of impropers and charges, because these are mentioned as expected_attrs in LammpsBase.

hmacdope · 2022-12-23T01:13:30Z

@hmacdope Thank you for your guiding! I tried to replicate what I saw in test_lammpsdata.py, and committed to the branch. When I run the test:

well done on getting the test files sorted out!

cd testsuite/MDAnalysisTests pytest --disable-pytest-warnings

It shows errors in some other modules, but not topology. However, I'm not sure that I've done everything correctly and the testing actually takes place.

The errors are coming from the issue with the density code, histogramdd normed argument replaced by density #3976 will fix it in due time so no need to worry about it yet.

~~You have imported the file but the testing is not yet taking place as you need to have asserts, but you have a good start~~

~~I will review to show you how to make the testing work~~

EDIT: Looks like the testing is working, sorry I didn't see that it was being done in the base class.

I'm also wondering, shall my datafile state the number of impropers and charges, because these are mentioned as expected_attrs in LammpsBase.

~~Lets see what happens when we add testing.~~

hmacdope · 2022-12-23T01:18:12Z

To test only your module go to MDAnalysistests/topology/ and run pytests test_lammpsdata.py

package/CHANGELOG

mglagolev · 2022-12-23T21:24:47Z

To test only your module go to MDAnalysistests/topology/ and run pytests test_lammpsdata.py

Done that.
My file understandably fails on test_expected_attributes: AssertionError: Missing expected attribute: charges
What shall I do? Create a different file with a different LAMMPS atom_style, which will include charges?

…add_pairij

IAlibay · 2022-12-23T22:15:44Z

testsuite/MDAnalysisTests/data/lammps/pairij_coeffs.data

@@ -0,0 +1,3228 @@
+LAMMPS data file via write_data, version 23 Jun 2022, timestep = 1000


Would it be possible to gzip or bzip2 this? i.e. is this something we support (I know we do for PDBs and similar, so I assume it should work here too?)

@IAlibay I'm sorry, I'm lost a little bit. How can I help at this point?

Compress the file(s) with bzip2 and store it as testsuite/MDAnalysisTests/data/lammps/pairij_coeffs.data.bz2. MDAnalysis will just read it the compressed file and we use up less space in the package and on disk.

Thank you! Recommitted with a zipped file.

IAlibay

I'm going to make that last one blocking, I'll unblock if it turns out to not be feasible.

This reverts commit 8269b20.

mglagolev · 2022-12-24T21:50:16Z

Thank you all, I've reverted the previous commit and recommitted with a zipped file.

IAlibay

Couple things from my side of things, thanks for bzipping that datafile.

package/CHANGELOG

testsuite/MDAnalysisTests/topology/test_lammpsdata.py

hmacdope · 2023-01-03T22:43:13Z

@mglagolev If you could fix the failing tests and the merge conflicts, we can move forward from there.
:)

…lev/mdanalysis into lammps_topology_add_pairij

IAlibay

Just the one quick thing from me.

IAlibay · 2023-01-04T18:20:53Z

package/CHANGELOG

@@ -13,11 +13,14 @@ The rules for this file:
  * release numbers follow "Semantic Versioning" http://semver.org

 ------------------------------------------------------------------------------
-??/??/?? IAlibay, pgbarletta
+


Suggested change

Please don't add the extra line here (we try to keep the format of this file reasonably strict to make life easier on releases).

IAlibay

Thanks!

mglagolev · 2023-01-04T19:18:43Z

@IAlibay @hmacdope
It seems that everything should be fine now, except for the darker lint test. But I thought maybe I should comply with the style already used in test_lammpsdata.py.
Thank you all for your big help with this small commit!

orbeckst · 2023-01-04T19:22:34Z

Thank you for your contribution @mglagolev !

Thank you all for your big help with this small commit!

Yes, as you can see, when working on software that's used by a few thousand researchers, there's a lot more to do for maintaining quality than just to add a few lines of code and hoping it will be fine. We do all this reviewing and insisting on tests with the goal to keep MDAnalysis a useful and maintainable piece of software for the foreseeable future.

IAlibay · 2023-01-04T19:30:48Z

testsuite/MDAnalysisTests/topology/test_lammpsdata.py

+    """Tests the reading of lammps .data topology file with a
+    PairIJ Coeffs section
+    """
+


Suggested change

No worries about the darker linting, as long as it's not breaching PEP8 it ends up just black trying to enforce its style.

That being said the above "whitespace on an empty line" is a PEP8 violation (I suspect the suggested edit here won't work well, essentially the empty line is fine, but it can't have any whitespaces on it)

Sorry about this, as @orbeckst explains re: keeping maintenance costs down, we end up requiring a lot of weird formatting rules that do tend to be rather unfriendly to new contributors :(

mglagolev · 2023-01-04T20:12:16Z

@orbeckst @IAlibay @hmacdope
I absolutely understand that this rigorousness is essential, and I'm very grateful for your immense work on this project.
Thanks for sort of onboarding me. I hope that I can contribute more in the future.

hmacdope

Fantastic work @mglagolev! Thanks for the contribution 😄 Many LAMMPS users will appreciate this one I'm sure. 👍

Add 'PairIJ Coeffs' section to the list of sections in LAMMPS parser.…

ee10b65

… This ensures correct processing of LAMMPS data files

github-actions bot added the Component-Topology label Dec 15, 2022

github-actions bot reviewed Dec 15, 2022

View reviewed changes

Updating AUTHORS and CHANGELOG

9277e7a

Adding a simple test

383423f

hmacdope requested changes Dec 19, 2022

View reviewed changes

mglagolev added 2 commits December 22, 2022 17:06

Revert "Adding a simple test"

2299045

This reverts commit 383423f. The test needs to be integrated into the test_lammpsdata.py instead of a separate script, and the sample datafile needs to be placed in the appropriate directory.

Adding test for LAMMPS PairIJ Coeffs file to test_lammpsdata

8269b20

mglagolev requested a review from hmacdope December 22, 2022 17:10

hmacdope requested changes Dec 23, 2022

View reviewed changes

package/CHANGELOG Show resolved Hide resolved

Merge remote-tracking branch 'upstream/develop' into lammps_topology_…

4edbdca

…add_pairij

mglagolev requested a review from hmacdope December 23, 2022 21:56

IAlibay reviewed Dec 23, 2022

View reviewed changes

IAlibay requested changes Dec 23, 2022

View reviewed changes

orbeckst added the format-LAMMPS label Dec 24, 2022

mglagolev added 2 commits December 25, 2022 00:32

Revert "Adding test for LAMMPS PairIJ Coeffs file to test_lammpsdata"

48b771e

This reverts commit 8269b20.

Adding the zipped test datafile and class

357dc9c

mglagolev requested review from IAlibay and removed request for hmacdope and IAlibay December 25, 2022 21:51

mglagolev requested review from IAlibay and hmacdope and removed request for IAlibay December 27, 2022 10:52

IAlibay requested changes Dec 27, 2022

View reviewed changes

package/CHANGELOG Show resolved Hide resolved

testsuite/MDAnalysisTests/topology/test_lammpsdata.py Show resolved Hide resolved

testsuite/MDAnalysisTests/topology/test_lammpsdata.py Show resolved Hide resolved

testsuite/MDAnalysisTests/topology/test_lammpsdata.py Show resolved Hide resolved

Implementing requested changes

9130850

mglagolev requested review from IAlibay and hmacdope and removed request for hmacdope and IAlibay December 28, 2022 17:17

mglagolev added 6 commits January 4, 2023 19:00

Bugfix to pass the tests; adding whitespace to pass darker lint

8d8786c

Bugfix

4b5885e

Merge branch 'develop' into lammps_topology_add_pairij

49421fe

Merging

b0beae3

Merge branch 'lammps_topology_add_pairij' of http://github.com/mglago…

6624fda

…lev/mdanalysis into lammps_topology_add_pairij

Remove a duplicate record due to a merging mess

4330ed4

IAlibay requested changes Jan 4, 2023

View reviewed changes

Fixing the format

4e134a6

IAlibay approved these changes Jan 4, 2023

View reviewed changes

IAlibay reviewed Jan 4, 2023

View reviewed changes

Fixing the PEP8 violation

d8fc45a

IAlibay requested a review from hmacdope January 5, 2023 20:24

hmacdope approved these changes Jan 6, 2023

View reviewed changes

hmacdope merged commit 08cf96f into MDAnalysis:develop Jan 6, 2023

IAlibay added the enhancement label Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'PairIJ Coeffs' section to the list of sections in LAMMPS parser #3959

Add 'PairIJ Coeffs' section to the list of sections in LAMMPS parser #3959

mglagolev commented Dec 15, 2022

github-actions bot left a comment

codecov bot commented Dec 15, 2022 •

edited

Loading

mglagolev commented Dec 15, 2022

hmacdope commented Dec 15, 2022

mglagolev commented Dec 16, 2022

mglagolev commented Dec 16, 2022

hmacdope left a comment

hmacdope Dec 19, 2022

hmacdope Dec 19, 2022

hmacdope Dec 19, 2022

mglagolev commented Dec 22, 2022

hmacdope commented Dec 23, 2022 •

edited

Loading

hmacdope commented Dec 23, 2022

mglagolev commented Dec 23, 2022

IAlibay Dec 23, 2022

mglagolev Dec 24, 2022

orbeckst Dec 24, 2022

mglagolev Dec 24, 2022

IAlibay left a comment

mglagolev commented Dec 24, 2022

IAlibay left a comment

hmacdope commented Jan 3, 2023

IAlibay left a comment

IAlibay Jan 4, 2023

IAlibay left a comment

mglagolev commented Jan 4, 2023

orbeckst commented Jan 4, 2023

IAlibay Jan 4, 2023 •

edited

Loading

mglagolev commented Jan 4, 2023

hmacdope left a comment

		import MDAnalysis as mda


		PAIRIJ_COEFFS_DATA = "PR3959_test.data"

		@@ -0,0 +1,3228 @@
		LAMMPS data file via write_data, version 23 Jun 2022, timestep = 1000

Add 'PairIJ Coeffs' section to the list of sections in LAMMPS parser #3959

Add 'PairIJ Coeffs' section to the list of sections in LAMMPS parser #3959

Conversation

mglagolev commented Dec 15, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 15, 2022 • edited Loading

Codecov Report

mglagolev commented Dec 15, 2022

hmacdope commented Dec 15, 2022

mglagolev commented Dec 16, 2022

mglagolev commented Dec 16, 2022

hmacdope left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mglagolev commented Dec 22, 2022

hmacdope commented Dec 23, 2022 • edited Loading

hmacdope commented Dec 23, 2022

mglagolev commented Dec 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

mglagolev commented Dec 24, 2022

IAlibay left a comment

Choose a reason for hiding this comment

hmacdope commented Jan 3, 2023

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

mglagolev commented Jan 4, 2023

orbeckst commented Jan 4, 2023

IAlibay Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

mglagolev commented Jan 4, 2023

hmacdope left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 15, 2022 •

edited

Loading

hmacdope commented Dec 23, 2022 •

edited

Loading

IAlibay Jan 4, 2023 •

edited

Loading