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

Update TIP5P comparison #1744

Merged
merged 11 commits into from
Nov 9, 2023
Merged

Update TIP5P comparison #1744

merged 11 commits into from
Nov 9, 2023

Conversation

mattwthompson
Copy link
Member

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

Merging #1744 (b9b4348) into main (575501a) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head b9b4348 differs from pull request most recent head d253c91. Consider uploading reports for the commit d253c91 to get more accurate results

Additional details and impacted files

@mattwthompson mattwthompson marked this pull request as ready for review October 13, 2023 18:12
@j-wags
Copy link
Member

j-wags commented Oct 24, 2023

I noticed that 1) a lot of notebook prose was deleted in this PR and 2) that this up-pins to interchange >=0.3.16 while #1754 to fix examples CI down-pins to interchange ==0.3.14, so I'm not sure whether this is ready to merge. I'm assigning @Yoshanuikabundi as a reviewer for this one (though if I misread this and this could supersede #1754 I can take it sooner)

@mattwthompson mattwthompson marked this pull request as draft October 24, 2023 03:29
@mattwthompson
Copy link
Member Author

There will need to be some updates to this notebook when the dust settles in Interchange, but it will look different than the current state - I'll re-open and re-trigger review with the web UI when it's done, nothing to worry about or work on here until then

Comment on lines -541 to +538
" [[0.0, 0.0, 0.0], [-0.7815, 0.5526, 0.0], [0.7815, 0.5526, 0.0]] * unit.angstrom\n",
" bond_length\n",
" * Quantity(\n",
" [\n",
" [0.0, 0.0, 0.0],\n",
" [-sin(theta / 2), cos(theta / 2), 0.0],\n",
" [sin(theta / 2), cos(theta / 2), 0.0],\n",
" ]\n",
" )\n",
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm convinced this is the cause of the years-long energy difference we've been observing

In [1]: from openff.units import *

In [2]: from math import *

In [3]: bond_length = Quantity(0.9572, unit.angstrom)

In [4]: theta = Quantity(104.52, unit.degree).to(unit.radian)

In [5]: bond_length * Quantity(
   ...:     [
   ...:         [0.0, 0.0, 0.0],
   ...:         [-sin(theta / 2), cos(theta / 2), 0.0],
   ...:         [sin(theta / 2), cos(theta / 2), 0.0],
   ...:     ],
   ...: )
Out[5]:
array([[ 0.        ,  0.        ,  0.        ],
       [-0.75695033,  0.58588228,  0.        ],
       [ 0.75695033,  0.58588228,  0.        ]]) <Unit('angstrom')>

@mattwthompson mattwthompson marked this pull request as ready for review November 7, 2023 17:24
@mattwthompson mattwthompson merged commit ee672ff into main Nov 9, 2023
@mattwthompson mattwthompson deleted the update-vsite-showcase branch November 9, 2023 18:37
mattwthompson pushed a commit that referenced this pull request Nov 10, 2023
* Use coordinates from modeller and add prose

* Add thumbnail
# 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.

3 participants