-
Notifications
You must be signed in to change notification settings - Fork 646
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
adds vsites as bonds between base particles and vsite #4318
base: develop
Are you sure you want to change the base?
Conversation
Hello @BartBruininks! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:
|
Linter Bot Results:Hi @BartBruininks! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
@BartBruininks said in #3168 (comment)
Adding them to bonds makes some sense to me but I'd appreciate the opinion of @richardjgowers @jbarnoud here. Once people agree, the next step would be to add tests. After we have tests, we can think about the TPR parser. |
I commented in #3168 (comment) |
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.
I summarized extensive discussion on the underlying implementation of vsites/dummies at #1954 (comment) .
We first need to properly define how vsites ought to be handled and then proceed with this PR. I will refer to discussions on #1954 as to what the data structure for vsites really should look like. It almost certainly will require work on implementing a new TopologyAttr. This could become part of this PR, given that this PR already solves the problem of reading the information from the TPR.
Partially addresses #1954
Changes made in this Pull Request:
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4318.org.readthedocs.build/en/4318/