HETATM record type written out #2880

mieczyslaw · 2020-07-30T17:38:27Z

Fixes #1753 (and may contribute to related #2826)

Changes made in this Pull Request:

added HETATM in fmt dictionary
conditional to write either ATOM or HETATM records based on atom.record_type

Tested on a PDB structure containing both ATOM and HETATM records and produces record types as expected.

PR Checklist

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

pep8speaks · 2020-07-30T17:38:32Z

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

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

Line 475:80: E501 line too long (93 > 79 characters)
Line 476:80: E501 line too long (97 > 79 characters)
Line 477:80: E501 line too long (98 > 79 characters)
Line 478:80: E501 line too long (92 > 79 characters)
Line 479:80: E501 line too long (97 > 79 characters)
Line 480:80: E501 line too long (97 > 79 characters)
Line 481:80: E501 line too long (97 > 79 characters)
Line 482:80: E501 line too long (95 > 79 characters)
Line 483:80: E501 line too long (97 > 79 characters)
Line 484:80: E501 line too long (93 > 79 characters)
Line 485:80: E501 line too long (95 > 79 characters)
Line 1193:80: E501 line too long (95 > 79 characters)
Line 1194:80: E501 line too long (97 > 79 characters)
Line 1195:80: E501 line too long (98 > 79 characters)

Comment last updated at 2020-08-14 20:32:37 UTC

IAlibay

Thanks for taking this on @mieczyslaw!

Just a quick first comment. Atoms don't always have a record_type attribute, actually in most cases they don't so we'll need to offer some kind of reasonable backup option there.

Also, just to point out #2826 is more about following the PDB standard than introducing the option to do HETATM/ATOM in writing. So we'd probably not want to close that issue at the same time here (unless we decide to implement a means to default "standard residues" to ATOM records here).

package/MDAnalysis/coordinates/PDB.py

codecov · 2020-07-30T19:40:58Z

Codecov Report

Merging #2880 into develop will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #2880      +/-   ##
===========================================
+ Coverage    92.93%   92.95%   +0.02%     
===========================================
  Files          187      187              
  Lines        24507    24579      +72     
  Branches      3185     3185              
===========================================
+ Hits         22775    22847      +72     
  Misses        1686     1686              
  Partials        46       46

Impacted Files	Coverage Δ
package/MDAnalysis/coordinates/PDB.py	`94.25% <100.00%> (+0.08%)`	⬆️
util.py	`89.47% <0.00%> (+0.07%)`	⬆️
coordinates/base.py	`95.12% <0.00%> (+0.26%)`	⬆️
auxiliary/base.py	`91.31% <0.00%> (+0.56%)`	⬆️

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 5e68617...e82974c. Read the comment docs.

IAlibay

A suggestion on how to clean things up so you don't have to check for the record type at every loop instance.

Additional things:

This would need to be documented, you'll want to add a versionchanged in the docstring, and something in the Note section.
We'll need some tests here. These should cover; a) checking for the presence of HETATM/ATOM records on reading a file (a read->write->read->assert would probably be sufficient), b) a check that the default ATOM write is happening (with a capture of the warning), c) a test checking what happens when you pass a non-normal record_type value.
Please update the CHANGELOG and AUTHORS accordingly.

package/MDAnalysis/coordinates/PDB.py

mieczyslaw · 2020-08-02T22:35:58Z

Thank you @IAlibay for your review. I addressed all your points, ran the tests, and pushed new commit. Please let me know if anything else needs to be improved.

IAlibay

Thanks for your work here @mieczyslaw

I realise I've added lots of comments & suggested changes, please do not let it discourage you. These are mainly "getting to know MDAnalysis" issues which will go away after a few pull requests.

package/MDAnalysis/coordinates/PDB.py

package/CHANGELOG

package/MDAnalysis/coordinates/PDB.py

mieczyslaw · 2020-08-05T16:39:11Z

Thanks for your work here @mieczyslaw

I realise I've added lots of comments & suggested changes, please do not let it discourage you. These are mainly "getting to know MDAnalysis" issues which will go away after a few pull requests.

I appreciate your efforts!

package/MDAnalysis/coordinates/PDB.py

IAlibay · 2020-08-06T09:15:03Z

@mieczyslaw, I left a series of comments regarding the tests, could you address these before I re-review please?

mieczyslaw · 2020-08-06T10:31:23Z

@mieczyslaw, I left a series of comments regarding the tests, could you address these before I re-review please?

if you mean the request to add three test cases, this is already added to testsuite/MDAnalysisTests/coordinates/test_pdb.py. I haven't seen any comments on that above, all were related to PDB.py.

IAlibay · 2020-08-06T11:11:46Z

if you mean the request to add three test cases, this is already added to testsuite/MDAnalysisTests/coordinates/test_pdb.py. I haven't seen any comments on that above, all were related to PDB.py.

@mieczyslaw It's possible that they aren't showing up on your screen because after a certain number of comments github will "hide the comments" see:

You need to click on "load more" to get the rest of the comments. If you can't see this, then please do have a look under the "files changed" tab, there should be comments there.

mieczyslaw · 2020-08-06T13:00:37Z

You need to click on "load more" to get the rest of the comments. If you can't see this, then please do have a look under the "files changed" tab, there should be comments there.

You're right! Thanks for your patience :)

If you could please follow up on my question re outfile fixture idea, I can proceed.

mieczyslaw · 2020-08-06T14:19:16Z

@IAlibay should I regenerate documentation as a part of this PR? when I checked the guide, it is a separate repo with its own setup process. thanks!

IAlibay · 2020-08-06T14:24:26Z

@IAlibay should I regenerate documentation as a part of this PR?

All you need to do here is check that the documentation changes you made build locally and behave as expected. See: https://userguide.mdanalysis.org/1.0.0/contributing_code.html#building-the-documentation

when I checked the guide, it is a separate repo with its own setup process.

I'm not sure what you mean here, maybe you can post the link you're referring to, so it's easier for me to let you know what is meant in the guide?

mieczyslaw · 2020-08-06T15:02:04Z

@IAlibay should I regenerate documentation as a part of this PR?

All you need to do here is check that the documentation changes you made build locally and behave as expected. See: https://userguide.mdanalysis.org/1.0.0/contributing_code.html#building-the-documentation

done, is there any reason that all html is zipped? e.g. index.html.gz

when I checked the guide, it is a separate repo with its own setup process.

I'm not sure what you mean here, maybe you can post the link you're referring to, so it's easier for me to let you know what is meant in the guide?

I can't find it now anymore, it was about cloning MDAnalysis/UserGuide, but not the case here as we are compiling dev docs.

I have regenerated and checked the docs.

Only CRYST1 points to v3.3 PDB standard, do you want me to move it to 3.2 (as the whole module says about compatibility with 3.2), or we can assume that it is compatible with 3.3 so I can modify all links to 3.3?

mieczyslaw · 2020-08-11T09:53:48Z

@IAlibay I see that merge conflict appeared due to another author added to Authors. Do you want me to rebase my branch?

IAlibay · 2020-08-12T13:25:59Z

Only CRYST1 points to v3.3 PDB standard, do you want me to move it to 3.2 (as the whole module says about compatibility with 3.2), or we can assume that it is compatible with 3.3 so I can modify all links to 3.3?

We probably should standardise things, as far as I'm aware there's no reason not to, but it might be worth just raising an issue detailing the 3.2/3.3 discrepancy. It can probably just be fixed in another PR if it takes too long for others to look at the issue.

I see that merge conflict appeared due to another author added to Authors.

Please do fix the merge conflict. You can just merge develop into this branch if you want (I'm going to squash merge so either way it doesn't really matter).

IAlibay

just a couple of things left I think

testsuite/MDAnalysisTests/coordinates/test_pdb.py

mieczyslaw · 2020-08-12T14:40:06Z

Only CRYST1 points to v3.3 PDB standard, do you want me to move it to 3.2 (as the whole module says about compatibility with 3.2), or we can assume that it is compatible with 3.3 so I can modify all links to 3.3?

We probably should standardise things, as far as I'm aware there's no reason not to, but it might be worth just raising an issue detailing the 3.2/3.3 discrepancy. It can probably just be fixed in another PR if it takes too long for others to look at the issue.

Done, #2906

I see that merge conflict appeared due to another author added to Authors.

Please do fix the merge conflict. You can just merge develop into this branch if you want (I'm going to squash merge so either way it doesn't really matter).

Done.

Thanks again for all your efforts @IAlibay !

mieczyslaw · 2020-08-13T22:07:23Z

@IAlibay I suppose the best would be if you resolve AUTHORS/CHANGELOG conflicts just before merging into develop (I previously resolved it, but if we wait for too long, this will happen again). Not sure about a failing codecov/project - seems to be related to other modules I haven't modified.

IAlibay · 2020-08-13T22:18:18Z

Will do @mieczyslaw, whilst we're here could you add the 3.2 -> 3.3 changes docstring changes? (I'm currently dealing with a series of Windows install issues but should have time to review & hopefully merge by the weekend).

mieczyslaw · 2020-08-13T23:15:09Z

Will do @mieczyslaw, whilst we're here could you add the 3.2 -> 3.3 changes docstring changes? (I'm currently dealing with a series of Windows install issues but should have time to review & hopefully merge by the weekend).

Done, all links checked and docs rebuilt.

IAlibay

Just the two docstring comments and I think we're done.

package/MDAnalysis/coordinates/PDB.py

IAlibay

lgtm! thanks for all your work @mieczyslaw

Fixes MDAnalysis#1753 and MDAnalysis#2906 ## Overview of work done in this PR 1) PDBWriter will now write out either `ATOM` or `HETATM` entries based on the contents of the atomgroup `record_types` attribute. If the `record_types` attribute is missing, writing will default to `ATOM` entries. 2) Updates the PDB documentation to refer to PDBv3.3 instead of PDBv3.2.

added HETATM fmt and conditional

e708aa7

IAlibay requested changes Jul 30, 2020

View reviewed changes

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

IAlibay mentioned this pull request Jul 30, 2020

Allow PDBWriter (and similar) to write record_types #1753

Closed

fix no attribute case

884d613

mieczyslaw requested a review from IAlibay July 30, 2020 20:00

IAlibay requested changes Aug 1, 2020

View reviewed changes

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

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

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

address reviewer's comments

46115ce

mieczyslaw requested a review from IAlibay August 2, 2020 23:30

IAlibay requested changes Aug 3, 2020

View reviewed changes

changes as suggested by the reviewer

9e646ec

mieczyslaw requested a review from IAlibay August 5, 2020 19:02

RMeli reviewed Aug 6, 2020

View reviewed changes

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

mieczyslaw added 3 commits August 6, 2020 14:41

additional comments addressed

0705ae8

use outfile fixture

09278fc

links sorted out

44f63c3

mieczyslaw requested a review from RMeli August 6, 2020 14:19

remove duplicated _CRYST1

96fbfbc

IAlibay requested changes Aug 12, 2020

View reviewed changes

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

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

mieczyslaw added 4 commits August 12, 2020 15:08

merge current develop, solve merge conflicts in AUTHORS

9c30e0f

address reviewer comments

31e58bb

match exception message

908746f

line lenght fix

d806658

mieczyslaw requested a review from IAlibay August 12, 2020 14:40

orbeckst assigned IAlibay Aug 12, 2020

change PDB version to 3.3

88802f9

mieczyslaw mentioned this pull request Aug 13, 2020

coordinates/PDB.py PDB 3.2 vs. 3.3 consistency #2906

Closed

mieczyslaw and others added 3 commits August 14, 2020 00:18

resolve conflicts

de923b2

Issue 2906

4b44ecb

Merge branch 'develop' into feature/write_hetatm_record_type_issue_1753

eff5759

IAlibay mentioned this pull request Aug 14, 2020

Azure pipelines failing (pytest-xdist) #2910

Closed

IAlibay requested changes Aug 14, 2020

View reviewed changes

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

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

mieczyslaw added 3 commits August 14, 2020 21:28

reviewer's request

6890a31

reviewer's request

d344006

reviewer's request

e82974c

mieczyslaw requested a review from IAlibay August 14, 2020 20:34

IAlibay approved these changes Aug 14, 2020

View reviewed changes

IAlibay merged commit 22612de into MDAnalysis:develop Aug 14, 2020

mieczyslaw deleted the feature/write_hetatm_record_type_issue_1753 branch August 16, 2020 15:03

fiona-naughton added Component-Writers enhancement labels Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HETATM record type written out #2880

HETATM record type written out #2880

mieczyslaw commented Jul 30, 2020 •

edited

Loading

pep8speaks commented Jul 30, 2020 •

edited

Loading

IAlibay left a comment

codecov bot commented Jul 30, 2020 •

edited

Loading

IAlibay left a comment

mieczyslaw commented Aug 2, 2020 •

edited

Loading

IAlibay left a comment •

edited

Loading

mieczyslaw commented Aug 5, 2020

IAlibay commented Aug 6, 2020

mieczyslaw commented Aug 6, 2020

IAlibay commented Aug 6, 2020

mieczyslaw commented Aug 6, 2020 •

edited

Loading

mieczyslaw commented Aug 6, 2020

IAlibay commented Aug 6, 2020

mieczyslaw commented Aug 6, 2020

mieczyslaw commented Aug 11, 2020

IAlibay commented Aug 12, 2020

IAlibay left a comment

mieczyslaw commented Aug 12, 2020

mieczyslaw commented Aug 13, 2020 •

edited

Loading

IAlibay commented Aug 13, 2020

mieczyslaw commented Aug 13, 2020

IAlibay left a comment

IAlibay left a comment

HETATM record type written out #2880

HETATM record type written out #2880

Conversation

mieczyslaw commented Jul 30, 2020 • edited Loading

PR Checklist

pep8speaks commented Jul 30, 2020 • edited Loading

Comment last updated at 2020-08-14 20:32:37 UTC

IAlibay left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 30, 2020 • edited Loading

Codecov Report

IAlibay left a comment

Choose a reason for hiding this comment

mieczyslaw commented Aug 2, 2020 • edited Loading

IAlibay left a comment • edited Loading

Choose a reason for hiding this comment

mieczyslaw commented Aug 5, 2020

IAlibay commented Aug 6, 2020

mieczyslaw commented Aug 6, 2020

IAlibay commented Aug 6, 2020

mieczyslaw commented Aug 6, 2020 • edited Loading

mieczyslaw commented Aug 6, 2020

IAlibay commented Aug 6, 2020

mieczyslaw commented Aug 6, 2020

mieczyslaw commented Aug 11, 2020

IAlibay commented Aug 12, 2020

IAlibay left a comment

Choose a reason for hiding this comment

mieczyslaw commented Aug 12, 2020

mieczyslaw commented Aug 13, 2020 • edited Loading

IAlibay commented Aug 13, 2020

mieczyslaw commented Aug 13, 2020

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

mieczyslaw commented Jul 30, 2020 •

edited

Loading

pep8speaks commented Jul 30, 2020 •

edited

Loading

codecov bot commented Jul 30, 2020 •

edited

Loading

mieczyslaw commented Aug 2, 2020 •

edited

Loading

IAlibay left a comment •

edited

Loading

mieczyslaw commented Aug 6, 2020 •

edited

Loading

mieczyslaw commented Aug 13, 2020 •

edited

Loading