-
Notifications
You must be signed in to change notification settings - Fork 696
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
PDBParser chainid segid / set groups #4965
base: develop
Are you sure you want to change the base?
PDBParser chainid segid / set groups #4965
Conversation
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.
Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on GitHub Discussions so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS
as part of this PR.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4965 +/- ##
===========================================
- Coverage 93.42% 93.42% -0.01%
===========================================
Files 177 189 +12
Lines 21859 22969 +1110
Branches 3078 3084 +6
===========================================
+ Hits 20422 21459 +1037
- Misses 986 1059 +73
Partials 451 451 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
might need to update the user guide as well if this PR is accepted |
Hi @lilyminium, I update the codes based on your comment in the issue #4948 (comment). let me know if it works for you. u = mda.Universe("test.pdb")
# try
u.guess_or_set_segments() # default guess chainIDs
# or
u.guess_or_set_segments(custom_segids=u.atoms.chainIDs) # or use any atomwise list you would like to set
# might possibly add it to the guess_TopologyAttrs, as you suggested below
# u.guess_TopologyAttrs(to_guess=["segids"], force_guess=["segids"])
# (or just let it stay outside, I am not sure which one is better) |
Thanks for trying that out, @yuyuan871111! As @tylerjereddy mentioned I think there are some design decisions still to be resolved. I'll reply on the parent issue just to centralize discussion |
I believe the design decisions have been finalized #4948 (comment). Can we proceed with the pull request? |
As we have discussed this improvement in #4948, would you mind giving some feedback from the perspective of the codes? I know it is a busy time for MDAnalysis core members (on GSoC application, MDAnalysis UserGuide/Document updates), but I hope this helpful improvement for MDAnlaysis users will not become a stale PR. Please let me know any suggestions and I am happy to clarify any unclear part (or improve codes). |
No concerns with tightening the SEGID columns that are read in PDB to 73-76, see #4948 (comment) |
Fixes #4948 #2874
Changes made in this Pull Request:
AtomGroup
/ResidueGroup
/SegmentGroup
in the universeforce_chainids_to_segids
inPDBReader
to forcibly reading the chain ID into the segment ID (PDB specific method)set_groups
to update the topology and groups in the universe when atomwise resids/segids is given (more generalized method across different file type)PR Checklist
package/CHANGELOG
file updated?package/AUTHORS
? (If it is not, add it!)Developers Certificate of Origin
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.
📚 Documentation preview 📚: https://mdanalysis--4965.org.readthedocs.build/en/4965/