Skip to content
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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

BartBruininks
Copy link
Contributor

@BartBruininks BartBruininks commented Oct 10, 2023

Partially addresses #1954

Changes made in this Pull Request:

  • ITP reads vsites

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

Developers certificate of origin


📚 Documentation preview 📚: https://mdanalysis--4318.org.readthedocs.build/en/4318/

@pep8speaks
Copy link

Hello @BartBruininks! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 312:29: E203 whitespace before ':'
Line 312:58: E261 at least two spaces before inline comment
Line 312:59: E262 inline comment should start with '# '
Line 313:29: E203 whitespace before ':'
Line 313:58: E261 at least two spaces before inline comment
Line 313:59: E262 inline comment should start with '# '
Line 314:29: E203 whitespace before ':'
Line 314:58: E261 at least two spaces before inline comment
Line 314:59: E262 inline comment should start with '# '
Line 315:29: E203 whitespace before ':'
Line 315:58: E261 at least two spaces before inline comment
Line 315:59: E262 inline comment should start with '# '
Line 316:29: E203 whitespace before ':'
Line 316:58: E261 at least two spaces before inline comment
Line 316:59: E262 inline comment should start with '# '
Line 357:1: W293 blank line contains whitespace
Line 358:42: E261 at least two spaces before inline comment
Line 361:57: W291 trailing whitespace
Line 363:61: W291 trailing whitespace
Line 366:70: W291 trailing whitespace
Line 367:68: W291 trailing whitespace
Line 378:61: W291 trailing whitespace
Line 380:1: W293 blank line contains whitespace
Line 382:5: E303 too many blank lines (2)
Line 382:42: E261 at least two spaces before inline comment
Line 385:57: W291 trailing whitespace
Line 387:61: W291 trailing whitespace
Line 390:70: W291 trailing whitespace
Line 391:68: W291 trailing whitespace
Line 402:61: W291 trailing whitespace
Line 404:1: W293 blank line contains whitespace
Line 405:42: E261 at least two spaces before inline comment
Line 408:57: W291 trailing whitespace
Line 410:61: W291 trailing whitespace
Line 413:70: W291 trailing whitespace
Line 414:68: W291 trailing whitespace
Line 425:61: W291 trailing whitespace
Line 427:1: W293 blank line contains whitespace
Line 428:42: E261 at least two spaces before inline comment
Line 431:57: W291 trailing whitespace
Line 433:61: W291 trailing whitespace
Line 436:70: W291 trailing whitespace
Line 437:68: W291 trailing whitespace
Line 448:61: W291 trailing whitespace
Line 450:1: W293 blank line contains whitespace
Line 451:42: E261 at least two spaces before inline comment
Line 454:57: W291 trailing whitespace
Line 456:61: W291 trailing whitespace
Line 459:70: W291 trailing whitespace
Line 460:68: W291 trailing whitespace
Line 464:34: E261 at least two spaces before inline comment
Line 471:61: W291 trailing whitespace

@github-actions
Copy link

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.

Code Location Outcome
main package ⚠️ Possible failure
testsuite ✅ Passed

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/6470613818/job/17567210229


Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

@orbeckst
Copy link
Member

@BartBruininks said in #3168 (comment)

More on the topic of this FIX, if we can agree that this is as intended (vsite interactions are added to bonds), I could also implement it for the TPR reader although I did not have a look at its complexity (I guess it is similar). The TPR reading still does not handle the vsites correctly.

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.

@jbarnoud
Copy link
Contributor

I commented in #3168 (comment)

Copy link
Member

@orbeckst orbeckst left a 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.

@orbeckst orbeckst self-assigned this Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants