-
Notifications
You must be signed in to change notification settings - Fork 45
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.
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
Fix PDB load_one for Cl atoms #284
Conversation
The same fix should work for any atom name, assuming the leading two characters define the symbol. This will not always work, e.g. PDB files often use If you want to make sure things work like they should, use PDB files with elements defined in the nearly last columns, instead of relying on atom names. We could add a warning for when the symbol is guessed from the atom name, just to clarify that one is potentially in trouble. |
Codecov Report
@@ Coverage Diff @@
## master #284 +/- ##
==========================================
- Coverage 95.01% 94.99% -0.03%
==========================================
Files 74 74
Lines 8811 8828 +17
Branches 1213 1216 +3
==========================================
+ Hits 8372 8386 +14
- Misses 191 193 +2
- Partials 248 249 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine to me. We just need to add a warning if the last column is missing since I have a feeling CA will happen
I've pushed a commit with the warnings. I have reorganized a few things to stay closer to the definitions in the PDB format. We should also consider opening an issue to rename some fields return by the PDB loader, eg. |
@FarnazH @evohringer @BradenDKelly This is ready to be merged as far as I am concerned. I'll merge this on May 15 unless there are new comments by then. |
Fixes #282