fix masses and charge bug in HOOMD-XML parser by CalCraven · Pull Request #2889 · MDAnalysis/mdanalysis

CalCraven · 2020-08-07T14:57:03Z

Fixes #2888

Changes made in this Pull Request:

Fix the HoomdXMLParser.py so that it properly reads in masses and charges instead of overriding all of those values with 0.0.

PR Checklist

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

pep8speaks · 2020-08-07T14:57:06Z

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

In the file package/MDAnalysis/coordinates/H5MD.py:

Line 572:80: E501 line too long (82 > 79 characters)
Line 814:80: E501 line too long (87 > 79 characters)

In the file package/MDAnalysis/transformations/rotate.py:

Line 43:80: E501 line too long (83 > 79 characters)
Line 45:80: E501 line too long (87 > 79 characters)
Line 51:80: E501 line too long (88 > 79 characters)
Line 62:80: E501 line too long (80 > 79 characters)
Line 74:80: E501 line too long (82 > 79 characters)
Line 154:1: W391 blank line at end of file

Comment last updated at 2020-08-12 16:24:27 UTC

codecov · 2020-08-07T15:59:18Z

Codecov Report

Merging #2889 into develop will increase coverage by 5.63%.
The diff coverage is 92.59%.

@@             Coverage Diff             @@
##           develop    #2889      +/-   ##
===========================================
+ Coverage    87.20%   92.83%   +5.63%     
===========================================
  Files          167      187      +20     
  Lines        21744    24520    +2776     
  Branches      3186     3192       +6     
===========================================
+ Hits         18961    22764    +3803     
+ Misses        2258     1710     -548     
+ Partials       525       46     -479

Impacted Files	Coverage Δ
package/MDAnalysis/transformations/rotate.py	`100.00% <ø> (ø)`
package/MDAnalysis/topology/HoomdXMLParser.py	`85.10% <50.00%> (+2.12%)`	⬆️
package/MDAnalysis/coordinates/H5MD.py	`94.55% <100.00%> (+70.41%)`	⬆️
package/MDAnalysis/topology/tpr/obj.py	`96.96% <0.00%> (-3.04%)`	⬇️
package/MDAnalysis/core/topology.py	`100.00% <0.00%> (ø)`
package/MDAnalysis/analysis/polymer.py	`100.00% <0.00%> (ø)`
package/MDAnalysis/coordinates/MMTF.py	`100.00% <0.00%> (ø)`
package/MDAnalysis/coordinates/NAMDBIN.py	`100.00% <0.00%> (ø)`
lib/__init__.py	`100.00% <0.00%> (ø)`
transformations/__init__.py	`100.00% <0.00%> (ø)`
... and 118 more

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 3ea6e43...17a07f4. Read the comment docs.

orbeckst

Thank you for pinpointing the error and providing a solution. This looks pretty good but before merging please address the following

add tests to testsuite/MDAnalysisTests/topology/test_hoodxml.py that shows that masses and charges are read correctly
remove the print() call
add an entry to CHANGELOG under "Fixes"
add yourself to AUTHORS

package/MDAnalysis/topology/HoomdXMLParser.py

annacraven · 2020-08-10T04:21:02Z

package/MDAnalysis/topology/HoomdXMLParser.py

            try:
                val = configuration.find(attrname)
-                vals = [mapper(el) for el in val.text.strip().split()]
+                vals = [mapper(el) for el in val.text.strip().split('\n')]


Wow that fixed it? What was wrong with splitting on any white space? I thought the problem was not doing .split()[1:]

That wasn't the issue. The issued ended up being with the plural vs singular terms for mass and charge being called at the wrong time.

…THORS and CHANGELOG

orbeckst

looking good.. please see comments

package/CHANGELOG

testsuite/MDAnalysisTests/topology/test_hoomdxml.py

orbeckst · 2020-08-10T21:15:03Z

Btw, the errors in your tests are due to #2890 .

orbeckst · 2020-08-11T17:23:42Z

Your tests fail because you need to import assert_almost_equal.

orbeckst · 2020-08-11T17:25:47Z

See https://travis-ci.com/github/MDAnalysis/mdanalysis/jobs/370755668#L2255-L2257

    def test_read_charges(self, top):
>       assert_almost_equal(top.charges.values, 0.0)
E       NameError: name 'assert_almost_equal' is not defined
testsuite/MDAnalysisTests/topology/test_hoomdxml.py:82: NameError

* Fixes MDAnalysis#2901 * Changes made in this Pull Request: - Added call to rotateby (instead of rotate) - Removed () from atom group

* fix MDAnalysis#2890 * added H5PYPicklable class (works in serial but not with driver="mpio" and MPI comm) * mock h5py so that the docs build even in the absence of h5py * tests (anything related to mpi is excluded from coverage) * update CHANGELOG

…alysis into CalCraven-update_xml_parser

- import assert_almost_equal - removed asserts that were not needed (and used deprecated constructs)

orbeckst · 2020-08-12T16:18:43Z

I fixed the test and merged the develop branch. If all the tests pass I will approve and merge.

orbeckst · 2020-08-12T23:11:49Z

Thank you for your fix @CalCraven , I also sent you an invitation to the MDAnaysis/contributors.

* Fixes #2888 * Fix the HoomdXMLParser.py so that it properly reads in masses and charges instead of overriding all of those values with 0.0. * add tests * update CHANGELOG * update AUTHORS Cherry-picked from 01bbf3c

CalCraven · 2020-08-13T16:11:26Z

Thanks for getting that to the finish line @orbeckst. This was my first PR so it was good to get that figured out.

orbeckst · 2020-08-13T16:13:09Z

Sure, I hope you'll contribute more!

* Fixes MDAnalysis#2888 * Fix the HoomdXMLParser.py so that it properly reads in masses and charges instead of overriding all of those values with 0.0. * add tests * update CHANGELOG * update AUTHORS

fix masses and charge bug

0e784b6

orbeckst changed the title ~~fix masses and charge bug~~ fix masses and charge bug in HOOMD-XML parser Aug 7, 2020

orbeckst requested changes Aug 7, 2020

View reviewed changes

package/MDAnalysis/topology/HoomdXMLParser.py Outdated Show resolved Hide resolved

orbeckst self-assigned this Aug 7, 2020

annacraven reviewed Aug 10, 2020

View reviewed changes

CalCraven added 3 commits August 10, 2020 13:14

remove extra lines of code, add unit tests, and add information to AU…

8efeca3

…THORS and CHANGELOG

fix merge errors

cbd7d09

update authors

0deedb9

orbeckst requested changes Aug 10, 2020

View reviewed changes

package/CHANGELOG Outdated Show resolved Hide resolved

package/CHANGELOG Outdated Show resolved Hide resolved

testsuite/MDAnalysisTests/topology/test_hoomdxml.py Outdated Show resolved Hide resolved

testsuite/MDAnalysisTests/topology/test_hoomdxml.py Outdated Show resolved Hide resolved

CalCraven added 3 commits August 11, 2020 10:28

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

165f75e

Follow format of CHANGELOG and add test_read_charges

58806dd

Merge branch 'develop' into update_xml_parser

156cbad

RMeli and others added 4 commits August 11, 2020 17:57

Fix rotateby documentation (MDAnalysis#2903)

5c0eb20

* Fixes MDAnalysis#2901 * Changes made in this Pull Request: - Added call to rotateby (instead of rotate) - Removed () from atom group

Merge branch 'update_xml_parser' of https://github.com/CalCraven/mdan…

5a2c792

…alysis into CalCraven-update_xml_parser

fixed hoomdxml test

6ee8018

- import assert_almost_equal - removed asserts that were not needed (and used deprecated constructs)

pep8 fixes

17a07f4

orbeckst approved these changes Aug 12, 2020

View reviewed changes

orbeckst added this to the 1.0.x milestone Aug 12, 2020

orbeckst merged commit 01bbf3c into MDAnalysis:develop Aug 12, 2020

CalCraven deleted the update_xml_parser branch August 13, 2020 16:11

fiona-naughton added defect Component-Readers labels Sep 26, 2023

Conversation

CalCraven commented Aug 7, 2020 • edited by orbeckst Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

pep8speaks commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-08-12 16:24:27 UTC

Uh oh!

codecov bot commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

annacraven Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

CalCraven Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

orbeckst commented Aug 10, 2020

Uh oh!

orbeckst commented Aug 11, 2020

Uh oh!

orbeckst commented Aug 11, 2020

Uh oh!

orbeckst commented Aug 12, 2020

Uh oh!

orbeckst commented Aug 12, 2020

Uh oh!

CalCraven commented Aug 13, 2020

Uh oh!

orbeckst commented Aug 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

CalCraven commented Aug 7, 2020 •

edited by orbeckst

Loading

pep8speaks commented Aug 7, 2020 •

edited

Loading

codecov bot commented Aug 7, 2020 •

edited

Loading