8000 Fix erroneous warnings with ANISOU records by swails · Pull Request #763 · ParmEd/ParmEd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix erroneous warnings with ANISOU records #763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Oct 1, 2016
Merged

Fix erroneous warnings with ANISOU records #763

merged 2 commits into from
Oct 1, 2016

Conversation

swails
Copy link
Contributor
@swails swails commented Sep 30, 2016

The mistake was very dumb. I was parsing the insertion code from ANISOU records in the 28th character field instead of the 27th.

This might also fix another bug when two residues with different insertion codes come right after each other but are the same kind of residue.

@swails
Copy link
Contributor Author
swails commented Sep 30, 2016

Fixes #762

@swails swails mentioned this pull request Sep 30, 2016
Closed
@swails
Copy link
Contributor Author
swails commented Oct 1, 2016

Should this have assertion?

Not necessary. This was a classic TDD flow. I wrote the test first, ran the code, and verified that it failed the way I expected it to fail (since setUp promotes PDBWarning to a fatal error).

I implemented the fix and the test passed. That's really all that needs to happen for that test to serve its purpose. There are lots of other tests that do detailed comparisons.

@swails swails merged commit 97bd56f into ParmEd:master Oct 1, 2016
@swails swails deleted the bugfix/anisou_inscode branch October 1, 2016 01:05
@hainm
Copy link
Contributor
hainm commented Oct 1, 2016

since setUp promotes PDBWarning to a fatal error

I see.

@dacase
Copy link
Contributor
dacase commented Dec 11, 2018

Same sort of typo-like misreading of the residue name in pdb.py. Line 518 reads:

rname = line[17:21].strip()

But the index should be 17:20. Compare line 250, where the residue name is extracted from an ATOM record. Since the only purpose of getting rname from the ANISOU card is to check that it matches the residue name from the assoicated ATOM record, this looks like a clear typo. I guessing ParmEd users don't see a lot of PDB files with ANISOU records, or you would be seeing errors.

Apologies for not just submitting a merge request: I've been trying to figure out how to fix lines 548-549, which don't work at all for me, and don't give any relevant information about which ANISOU card is misbehaving. But am sort of swamped with other things right now.

@dacase
Copy link
Contributor
dacase commented Mar 21, 2019

This is still a definite problem, and I don't think things are really fixed. But the complexity of the python code at this point is prevented me from easily proposing a pull request. Some notes:

  • The (presumed) error in line 518 of pdb.py (see previous comment) is still there.
  • When a mismatch is found, the error message printed to stdout is garbled. Here is an example:
/Users/case/phenix/conda_base/lib/python2.7/site-packages/parmed/formats/pdb.py:552: PDBWarning: ANISOU record does not match previous atom
  'atom', PDBWarning)

I'm not sure what 'atom' part right before "PDBWarning" is supposed to be here. But the absence of any information about which ANISOU card raised the error makes it really difficult to localize the problem. (This part of the comment amplifies the somewhat vague complaint about lines 548-549. It would be neat to (optionally?) print out the offending ANISOU line itself, but I don't know the correct way to do that in this context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@swails @hainm @dacase
0