Skip to content
New issue

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

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

Already on GitHub? # to your account

Topology.from_openmm raises informative error if origin PDB lacks CONECT #357

Merged
merged 2 commits into from
Jun 18, 2019

Conversation

j-wags
Copy link
Member

@j-wags j-wags commented Jun 14, 2019

@j-wags j-wags requested a review from andrrizzi June 14, 2019 18:08
@j-wags j-wags changed the title Top.from_openmm raises informative error if origin PDB lacks CONECT Topology.from_openmm raises informative error if origin PDB lacks CONECT Jun 14, 2019
msg += 'This would be a very unusual molecule to try and parameterize, ' \
'and it is likely that the data source it was read from does not ' \
'contain connectivity information. If this molecule is coming from ' \
'PDB, please ensure that the file contains CONECT lines.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're called CONECT "records" or "cards", I believe.
We might also cite the PDB format: https://www.wwpdb.org/documentation/file-format-content/format33/sect10.html

The important part of the PDB format is:

CONECT records are mandatory for HET groups (excluding water) and for other bonds not specified in the standard residue connectivity table.

That's the key here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added link to the PDB spec. Happy to do my part in the war for the PDB file format :-)

Copy link
Member Author

@j-wags j-wags Jun 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also changed wording to "CONECT records"

Copy link
Collaborator

@andrrizzi andrrizzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks so much!

@codecov-io
Copy link

Codecov Report

Merging #357 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #357      +/-   ##
==========================================
+ Coverage   75.95%   75.97%   +0.01%     
==========================================
  Files          17       17              
  Lines        5431     5435       +4     
==========================================
+ Hits         4125     4129       +4     
  Misses       1306     1306
Impacted Files Coverage Δ
openforcefield/topology/topology.py 73.45% <100%> (+0.14%) ⬆️

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 cc1d5eb...817e37f. Read the comment docs.

@j-wags j-wags merged commit f308bde into master Jun 18, 2019
@j-wags j-wags deleted the pdb-missing-conect-msg branch June 18, 2019 20:05
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide more useful error message if topology.from_openmm has connectivity problems
4 participants