Make CRD EXT format optional #3635

mdpoleto · 2022-04-13T19:02:07Z

Fixes issue #3605

Changes made in this Pull Request:

Inclusion of an boolean crdext input for the CRDWriter.write() function.

PR Checklist

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

pep8speaks · 2022-04-13T19:02:11Z

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

In the file testsuite/MDAnalysisTests/coordinates/test_crd.py:

Line 27:17: E231 missing whitespace after ','
Line 91:17: E127 continuation line over-indented for visual indent
Line 93:13: E128 continuation line under-indented for visual indent
Line 94:25: E127 continuation line over-indented for visual indent
Line 95:44: W291 trailing whitespace
Line 96:13: E128 continuation line under-indented for visual indent
Line 97:25: E127 continuation line over-indented for visual indent
Line 98:80: E501 line too long (101 > 79 characters)

Comment last updated at 2022-04-18 18:42:36 UTC

codecov · 2022-04-13T19:23:16Z

Codecov Report

Merging #3635 (2d3819e) into develop (c0a4f61) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #3635      +/-   ##
===========================================
+ Coverage    94.25%   94.28%   +0.03%     
===========================================
  Files          190      190              
  Lines        24791    24792       +1     
  Branches      3340     3340              
===========================================
+ Hits         23367    23376       +9     
+ Misses        1376     1368       -8     
  Partials        48       48

Impacted Files	Coverage Δ
package/MDAnalysis/coordinates/CRD.py	`92.15% <100.00%> (+6.01%)`	⬆️
package/MDAnalysis/topology/CRDParser.py	`94.44% <0.00%> (+3.70%)`	⬆️

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 c0a4f61...2d3819e. Read the comment docs.

orbeckst

Thanks for the PR @mdpoleto . I have a number of initial comments.

Why do we need the extended kwarg in write() and init? Isn't init sufficient? Right now I'd remove the kwarg from write() but if you can convince me that it should be there I'll change my mind. Is there a specific reason why it needs to be in two places or do other writers do similar things?
The code can be simplified, see comments (you can accept the suggestions to make your life easier).
Please add tests for the new functionality.
Please update CHANGELOG.
Add yourself to AUTHORS.

package/MDAnalysis/coordinates/CRD.py

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

mdpoleto · 2022-04-14T13:54:09Z

@orbeckst Thanks for your comments and patience. This is my first PR to a big code like MDA, so I am still learning how to navigate the process. I am not sure how to add test cases for this new functionality. Could you provide some tips to that?

orbeckst · 2022-04-14T16:35:28Z

Contributing to a medium sized package such as MDAnalysis is certainly a learning experience. In the User Guide we have a section Contributing to the main codebase that you should have a look at. We're trying to help, and we thank you for your patience in advance. The process can take a while because we strive to maintain high code quality so there will invariably be multiple rounds of reviews, often from different developers. We aren't making petty requests but we are all trying our best to make our code useful and robust for the few thousand users that rely on it for their work.

Testing is therefore incredibly important; the User Guide has a section on testing. The CRD format can be used both for topology information (through the CRDParser) and as a coordinate CRDReader and CRDWriter. You are modifying the CRDWriter so you need to add tests that check your new functionality. These tests are located in testsuite/MDAnalysisTests/coordinates/test_crd.py. Look for tests for writing. Then start by adding your own test, modelled on the existing one, where you use your new feature. Check in your test that the output is correct.

During development I run tests locally using pytest — the User Guide tells you more.

When you push your changes to the branch, have a look at the Checks and the coverage. It can tell you which lines have not been tested.

package/CHANGELOG

mdpoleto · 2022-04-14T21:17:54Z

@orbeckst Thanks for all the info. I did read the "Contributing to the main codebase" section, which was very helpful. The testing section now was very helpful.

I included a test for the extended format and it ran just fine locally. I included it on my last commit. If anything else is needed, please let me know.

testsuite/MDAnalysisTests/coordinates/test_crd.py

…to MDAnalysis-develop

orbeckst

Looks really good! I have a minor doc comment and I'd like to see a second test that shows that we can read the extended file that we wrote. It would look similar to the test you wrote but now you also read back the outfile and assert that you get the right number of atoms, residues, and perhaps the center of geometry of all positions, and the total charge of the system (or something like that — you get the idea, test in aggregate so that you see if something is not read properly).

package/MDAnalysis/coordinates/CRD.py

testsuite/MDAnalysisTests/coordinates/test_crd.py

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

richardjgowers · 2022-04-16T10:24:11Z

testsuite/MDAnalysisTests/coordinates/test_crd.py

+        #return str(tmpdir) + '/out.crd'
+        return os.path.join(str(tmpdir), 'test.crd')


why this change? iirc tmpdir is a Path object so should work ok as is?

From what I understood from @tylerjereddy 's comment, using this construct is preferred since it does not leave artifacts behind. See full discussion at: #3635 (comment)

This was indeed what @tylerjereddy suggested.

Maybe @richardjgowers was thinking to use

return str(tmpdir / 'test.crd')

We do need a str for Universe as we cannot use py.path or pathfile objects yet.

By the way, please remove commented out code.

mdpoleto · 2022-04-18T16:21:32Z

@orbeckst at the time of my last commit all checks passed but now there seems to be a conflict with AUTHORS. Should I address it?

orbeckst · 2022-04-18T17:17:24Z

@mdpoleto yes please resolve the conflict — just add your name at the end of the AUTHORS list.

We just had another PR merged that collided with yours. That's not uncommon.

…to issue-3605

orbeckst

please see comments, sorry to be short

testsuite/MDAnalysisTests/coordinates/test_crd.py

package/AUTHORS

testsuite/MDAnalysisTests/coordinates/test_crd.py

mdpoleto · 2022-04-18T18:25:26Z

@orbeckst I believe I addressed them all. Let me know if anything else is needed

orbeckst

All good. Thank you for your contribution!

I had some minor comments that I am adding to the PR.

When the tests come back all clear, I will squash-merge the PR.

testsuite/MDAnalysisTests/coordinates/test_crd.py

package/MDAnalysis/coordinates/CRD.py

orbeckst · 2022-04-18T20:19:54Z

Congratulations on your first contribution, @mdpoleto !

I'll send you an invite for the org so that you're also listed as a Contributor, if you accept.

mdpoleto · 2022-04-18T21:19:53Z

Thanks for all the help and guidance @orbeckst

@tbengtsen

Update of AUTHORS and CHANGELOG with inferred author contributions. * Removed duplicate mattwthompson in 0.20.0 changelog entry.: mattwthompson was placed twice by accident, this removes this duplication. * Addition of missing authors. An retrospective analysis of the authors found via `git shortlog -s -n --email --branches="develop"` found several commits by authors which were not present in the `AUTHORS.md` file. - Zhenbo Li: commited via PR: Started tests for gnm. #803 and Make Travis run tests on OSX. #771, - Jenna M. Swarthout via PR Update CoC according to suggestions from current CoC committee #4289 and Point to new reporting form link (owned by conduct@mdanalysis.org) #4298, - Bradley Dice via PR Fix GSD Windows compatibility #2384 , - David Minh via PR #2668 There seemed to be no indications in the above mentioned PRs that the author did not want to be in the authors file, it looks like it was just forgotten. * Addition of missing entries from the changelog Continued cross referencing of the git shortlog output but also accounting for the changelog identified several individuals that had not been included in the changelog entries for the release they contributed under. They were added to the relevant entry of the changelog based on the merge commit date. Please note that for Tone Bengsten, we a) had no github handle (so they were assigned @tbengtsen), and b) no specific commit. Given we know that this individual was including alongside the encore merge, they were assigned to the 0.16.0 release. * Update CHANGELOG * PR #1218 * PR #1284 and #1408 * PR #4109 * based on git history * PRs #803 and #771 (author addition, changelog addition) * PR #2255 and #2221 * PR #1225 * PR #4289 and #4298 * PR #4031 * PR #4085 * PR #3635 * PR #2356 * PR #2559 * No GH handle - Encore author addition @tbengtsen * PR #4184 * PR #2614 * PR #2219 * PR #2384 * PR #2668 * Add missing entry for Jenna Thanks to @fiona-naughton for helping out with digging into this data :) Co-authored-by: Fiona Naughton <fiona@mdanalysis.org> Co-authored-by: Oliver Beckstein <orbeckst@mdanalysis.org>

mdpoleto added 3 commits April 12, 2022 20:06

CRD extended format an option for small systems

6d6a55e

make CRD EXT format optional

9c64f40

make CRD EXT format optional

beb6159

github-actions bot added the Component-Readers label Apr 13, 2022

make CRD EXT format optional

1aec9f8

orbeckst requested changes Apr 14, 2022

View reviewed changes

mdpoleto and others added 5 commits April 14, 2022 09:21

Update package/MDAnalysis/coordinates/CRD.py

ca4b46b

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

Update package/MDAnalysis/coordinates/CRD.py

6437458

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

Update package/MDAnalysis/coordinates/CRD.py

a2d0353

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

Update package/MDAnalysis/coordinates/CRD.py

53494d5

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

Simplifying CRD extended code

430c4b8

orbeckst reviewed Apr 14, 2022

View reviewed changes

package/CHANGELOG Show resolved Hide resolved

mdpoleto added 2 commits April 14, 2022 17:11

Include tests for CRD ext format

6a4f04b

Include tests for CRD ext format

714945e

Include tests for CRD ext format

f304346

tylerjereddy reviewed Apr 15, 2022

View reviewed changes

testsuite/MDAnalysisTests/coordinates/test_crd.py Outdated Show resolved Hide resolved

mdpoleto added 5 commits April 15, 2022 10:23

Improve tests for CRD EXT format

cf061f1

to be overwritten

1419ab9

Merge branch 'develop' of https://github.com/MDAnalysis/mdanalysis in…

b10ec16

…to MDAnalysis-develop

Merge branch 'MDAnalysis-develop' into issue-3605

eedd551

update AUTHORS

c3cf075

orbeckst requested changes Apr 15, 2022

View reviewed changes

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

testsuite/MDAnalysisTests/coordinates/test_crd.py Outdated Show resolved Hide resolved

mdpoleto and others added 3 commits April 15, 2022 22:25

Update package/MDAnalysis/coordinates/CRD.py

9077a6d

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

Add test to read EXT format

4765c8f

Improve test to read EXT format

b11b6c4

richardjgowers reviewed Apr 16, 2022

View reviewed changes

mdpoleto added 2 commits April 18, 2022 13:28

Merge branch 'develop' of https://github.com/MDAnalysis/mdanalysis in…

53c93e3

…to issue-3605

Update AUTHORS

30ffb41

orbeckst requested changes Apr 18, 2022

View reviewed changes

mdpoleto added 2 commits April 18, 2022 13:36

Update AUTHORS and CRD read tests

276b0b7

Update AUTHORS and CRD read tests

04879c3

orbeckst approved these changes Apr 18, 2022

View reviewed changes

testsuite/MDAnalysisTests/coordinates/test_crd.py Outdated Show resolved Hide resolved

testsuite/MDAnalysisTests/coordinates/test_crd.py Outdated Show resolved Hide resolved

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

Apply suggestions from code review

2d3819e

orbeckst self-assigned this Apr 18, 2022

orbeckst merged commit 32de86f into MDAnalysis:develop Apr 18, 2022

orbeckst linked an issue Apr 18, 2022 that may be closed by this pull request

Make CRD (CHARMM card file) EXT format optional for smaller systems. #3605

Closed

orbeckst mentioned this pull request Apr 23, 2022

Make CRD (CHARMM card file) EXT format optional for smaller systems. #3605

Closed

IAlibay added the enhancement label Sep 25, 2023

IAlibay added a commit that referenced this pull request Dec 17, 2023

PR #3635

4cfe268

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make CRD EXT format optional #3635

Make CRD EXT format optional #3635

mdpoleto commented Apr 13, 2022 •

edited

pep8speaks commented Apr 13, 2022 •

edited

codecov bot commented Apr 13, 2022 •

edited

orbeckst left a comment

mdpoleto commented Apr 14, 2022

orbeckst commented Apr 14, 2022

mdpoleto commented Apr 14, 2022 •

edited

orbeckst left a comment

richardjgowers Apr 16, 2022

mdpoleto Apr 16, 2022 •

edited

orbeckst Apr 18, 2022

mdpoleto commented Apr 18, 2022

orbeckst commented Apr 18, 2022

orbeckst left a comment

mdpoleto commented Apr 18, 2022

orbeckst left a comment

orbeckst commented Apr 18, 2022

mdpoleto commented Apr 18, 2022

		#return str(tmpdir) + '/out.crd'
		return os.path.join(str(tmpdir), 'test.crd')

Make CRD EXT format optional #3635

Make CRD EXT format optional #3635

Conversation

mdpoleto commented Apr 13, 2022 • edited

PR Checklist

pep8speaks commented Apr 13, 2022 • edited

Comment last updated at 2022-04-18 18:42:36 UTC

codecov bot commented Apr 13, 2022 • edited

Codecov Report

orbeckst left a comment

Choose a reason for hiding this comment

mdpoleto commented Apr 14, 2022

orbeckst commented Apr 14, 2022

mdpoleto commented Apr 14, 2022 • edited

orbeckst left a comment

Choose a reason for hiding this comment

richardjgowers Apr 16, 2022

Choose a reason for hiding this comment

mdpoleto Apr 16, 2022 • edited

Choose a reason for hiding this comment

orbeckst Apr 18, 2022

Choose a reason for hiding this comment

mdpoleto commented Apr 18, 2022

orbeckst commented Apr 18, 2022

orbeckst left a comment

Choose a reason for hiding this comment

mdpoleto commented Apr 18, 2022

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst commented Apr 18, 2022

mdpoleto commented Apr 18, 2022

mdpoleto commented Apr 13, 2022 •

edited

pep8speaks commented Apr 13, 2022 •

edited

codecov bot commented Apr 13, 2022 •

edited

mdpoleto commented Apr 14, 2022 •

edited

mdpoleto Apr 16, 2022 •

edited