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

Jump to bottom

Fix incorrect assignment of TIP3P parameters in Sage #1200

Merged
merged 7 commits into from
Feb 26, 2022
Merged

Fix incorrect assignment of TIP3P parameters in Sage #1200

merged 7 commits into from
Feb 26, 2022

Conversation

mattwthompson
Copy link
Member

@mattwthompson mattwthompson commented Feb 25, 2022 •
edited
Loading

Fixes #1199

I didn't push in the right order to get CI runs failing with the added test, but they work (i.e. appropriately fail and pass before and after) locally and anybody can verify that by pulling it down.

5fd1517 does not obviously do anything not covered in multiple other unit tests, but it's cheap to add

  • change the line linked above to have LibraryChargeHandler.find_matches NOT uniquify by default
  • add a test that ensures multiple variants of TIP3P parameters are applied correctly
  • Tag issue being addressed
  • Add tests
  • Update docstrings/documentation, if applicable
  • Lint codebase
  • Update changelog

@codecov
Copy link

codecov bot commented Feb 25, 2022 •
edited
Loading

Codecov Report

Merging #1200 (8bb2aea) into master (b1d8e7e) will decrease coverage by 0.36%.
The diff coverage is 100.00%.

@mattwthompson mattwthompson marked this pull request as ready for review February 25, 2022 20:58
@j-wags
Copy link
Member

j-wags commented Feb 25, 2022

Thanks for the quick action, @mattwthompson! Seeing how quickly unintended side effects can spread, I'm going to take a few minutes to review this carefully.

j-wags
j-wags approved these changes Feb 25, 2022
View reviewed changes
Copy link
Member

@j-wags j-wags left a comment

Choose a reason for hiding this comment

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

Thanks for opening this, @mattwthompson

I made some direct changes and am pretty sure that the current state is cleaner - looking around, we already had two tests for TIP3P LibraryCharge application, it would have been a bit much to add a third :-)

I think this is ready to merge - Could you update the release notes? Then we can cut 0.10.3 on Monday.

@mattwthompson
Copy link
Member Author

mattwthompson commented Feb 25, 2022 •
edited
Loading

I definitely wrote them in the first set of changes ... but they're also not there, so my git foo must have botched that. Oh well.

I'm not going to dwell on the text of the change here and delay hitting the green button here - we can revise while making the release.

Will merge when green!

@mattwthompson mattwthompson merged commit 485414e into master Feb 26, 2022
@mattwthompson mattwthompson deleted the fix-tip3p-sage branch February 26, 2022 05:27
@mattwthompson mattwthompson mentioned this pull request Mar 2, 2022
Closed
4 tasks
mattwthompson added a commit that referenced this pull request Mar 17, 2022
@mattwthompson @j-wags
Fix incorrect assignment of TIP3P parameters in Sage (#1200)
22ff547 
* Add test for TIP3P charges with Sage

* Do not uniquify SMARTS in LibraryChargeHandler

* Also test standalone TIP3P file

* Consolidate tip3p charge tests

* black

* Update release history

Co-authored-by: j-wags <jwagnerjpl@gmail.com>
mattwthompson added a commit that referenced this pull request Mar 23, 2022
@mattwthompson @j-wags
Fix incorrect assignment of TIP3P parameters in Sage (#1200) (#1220)
f297b64 
* Add test for TIP3P charges with Sage

* Do not uniquify SMARTS in LibraryChargeHandler

* Also test standalone TIP3P file

* Consolidate tip3p charge tests

* black

* Update release history

Co-authored-by: j-wags <jwagnerjpl@gmail.com>

Co-authored-by: j-wags <jwagnerjpl@gmail.com>
mattwthompson added a commit that referenced this pull request Mar 24, 2022
@mattwthompson
Squashed commit of the following:
530d4b9 
commit d34fe857e7cf28c312141bca58ff3b81173eecb0
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 24 13:27:43 2022 -0500

    Fix bad merge

commit 7d309f2
Merge: 9273dbb b373e40
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 24 11:42:15 2022 -0500

    Merge remote-tracking branch 'upstream/add-topology' into add-topology

commit 9273dbb
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 24 10:44:12 2022 -0500

    Fix mypy

commit 4ec3ea7
Merge: b8a5793 45fb642
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 24 10:41:13 2022 -0500

    Merge remote-tracking branch 'upstream/topology-biopolymer-refactor' into add-topology

commit 45fb642
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Wed Mar 23 21:34:20 2022 -0500

    Raise descriptive error when OpenEye stereochemistry assumptions fail (#1218)

commit fc3d637
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Wed Mar 23 21:33:52 2022 -0500

    Performance improvements in from_openeye (#1217)

    OpenMM quantities are generally as quick and lightweight as
    corresponding NumPy arrays, but Pint units tend to be slower. Most of
    these changes simply defer unit tagging until a final step, carrying
    values with implicit units in some internal functions where the units
    are known and can be expected not to vary.

commit b373e40
Merge: b8a5793 f297b64
Author: Jeff Wagner <jwagnerjpl@gmail.com>
Date:   Wed Mar 23 16:04:17 2022 -0700

    Merge branch 'topology-biopolymer-refactor' into add-topology

commit f297b64
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Wed Mar 23 17:21:16 2022 -0500

    Fix incorrect assignment of TIP3P parameters in Sage (#1200) (#1220)

    * Add test for TIP3P charges with Sage

    * Do not uniquify SMARTS in LibraryChargeHandler

    * Also test standalone TIP3P file

    * Consolidate tip3p charge tests

    * black

    * Update release history

    Co-authored-by: j-wags <jwagnerjpl@gmail.com>

    Co-authored-by: j-wags <jwagnerjpl@gmail.com>

commit a8d8b09
Author: Jeff Wagner <jwagnerjpl@gmail.com>
Date:   Mon Mar 21 14:44:21 2022 -0700

    Resolve conflicts (#1224)

commit 8d4a0a8
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Fri Mar 11 18:27:50 2022 -0600

    Remove un-used Topology attributes (#1213)

    * Remove un-used Topology attributes

    * Lint

    * Update release history

commit ba879b2
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Fri Mar 11 18:26:50 2022 -0600

    Remove some internal calls to deprecated API points (#1204)

    * Remove some internal calls to deprecated API points

    * Fixes

    * Add test that deprecated Topology methods still exist

    * Fix broken list population

commit 59f35b8
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Fri Mar 11 17:45:05 2022 -0600

    Avoid silent/implicit units checks in Topology.to_file, update docstring (#1207)

commit 46893b0
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Fri Mar 11 16:15:22 2022 -0600

    Add Electrostatics tag to `test_forcefields/tip3p.offxml` (#1188)

    * Add Electrostatics section to data/test_forcefields/tip3p.offxml

    * Update release history

commit 0c42148
Author: Jeff Wagner <jwagnerjpl@gmail.com>
Date:   Fri Mar 11 13:43:59 2022 -0800

    fix #837 (#1214)

commit 0cb3a72
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 10 14:34:37 2022 -0600

    Fix formatting in a `.assign_partial_charges` docstring (#1212)

commit b8a5793
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Wed Feb 9 11:58:02 2022 -0600

    Remove old mendeleev import

commit 373262a
Merge: 31c7755 0beef75
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Wed Feb 9 11:38:27 2022 -0600

    Merge remote-tracking branch 'upstream/topology-biopolymer-refactor' into add-topology

commit 31c7755
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Wed Feb 9 11:30:23 2022 -0600

    Update Topology.__add__/__iadd__, add tests, fix annotations

commit fc12de1
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Tue Feb 8 15:33:41 2022 -0600

    Add first implementation of Topoogy.__add__

commit 0321d99
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 12:30:12 2022 -0600

    Remove Atom.element

commit a2e7bf9
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 12:25:03 2022 -0600

    Fix virtual site creation

commit a383a0a
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 11:26:19 2022 -0600

    Fix setting masses while adding OpenMM particles

commit 37e86db
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 10:20:30 2022 -0600

    Fix more typos

commit 6555475
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 09:33:13 2022 -0600

    Fix typos, tests

commit c529ee0
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Wed Jan 26 18:49:04 2022 -0600

    Draft a refactor to openff.units.elements
mattwthompson added a commit that referenced this pull request May 11, 2022
@mattwthompson
Add Topology.__add__ (#1194)
a5b6000 
* Squashed commit of the following:

commit d34fe857e7cf28c312141bca58ff3b81173eecb0
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 24 13:27:43 2022 -0500

    Fix bad merge

commit 7d309f2
Merge: 9273dbb b373e40
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 24 11:42:15 2022 -0500

    Merge remote-tracking branch 'upstream/add-topology' into add-topology

commit 9273dbb
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 24 10:44:12 2022 -0500

    Fix mypy

commit 4ec3ea7
Merge: b8a5793 45fb642
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 24 10:41:13 2022 -0500

    Merge remote-tracking branch 'upstream/topology-biopolymer-refactor' into add-topology

commit 45fb642
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Wed Mar 23 21:34:20 2022 -0500

    Raise descriptive error when OpenEye stereochemistry assumptions fail (#1218)

commit fc3d637
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Wed Mar 23 21:33:52 2022 -0500

    Performance improvements in from_openeye (#1217)

    OpenMM quantities are generally as quick and lightweight as
    corresponding NumPy arrays, but Pint units tend to be slower. Most of
    these changes simply defer unit tagging until a final step, carrying
    values with implicit units in some internal functions where the units
    are known and can be expected not to vary.

commit b373e40
Merge: b8a5793 f297b64
Author: Jeff Wagner <jwagnerjpl@gmail.com>
Date:   Wed Mar 23 16:04:17 2022 -0700

    Merge branch 'topology-biopolymer-refactor' into add-topology

commit f297b64
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Wed Mar 23 17:21:16 2022 -0500

    Fix incorrect assignment of TIP3P parameters in Sage (#1200) (#1220)

    * Add test for TIP3P charges with Sage

    * Do not uniquify SMARTS in LibraryChargeHandler

    * Also test standalone TIP3P file

    * Consolidate tip3p charge tests

    * black

    * Update release history

    Co-authored-by: j-wags <jwagnerjpl@gmail.com>

    Co-authored-by: j-wags <jwagnerjpl@gmail.com>

commit a8d8b09
Author: Jeff Wagner <jwagnerjpl@gmail.com>
Date:   Mon Mar 21 14:44:21 2022 -0700

    Resolve conflicts (#1224)

commit 8d4a0a8
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Fri Mar 11 18:27:50 2022 -0600

    Remove un-used Topology attributes (#1213)

    * Remove un-used Topology attributes

    * Lint

    * Update release history

commit ba879b2
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Fri Mar 11 18:26:50 2022 -0600

    Remove some internal calls to deprecated API points (#1204)

    * Remove some internal calls to deprecated API points

    * Fixes

    * Add test that deprecated Topology methods still exist

    * Fix broken list population

commit 59f35b8
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Fri Mar 11 17:45:05 2022 -0600

    Avoid silent/implicit units checks in Topology.to_file, update docstring (#1207)

commit 46893b0
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Fri Mar 11 16:15:22 2022 -0600

    Add Electrostatics tag to `test_forcefields/tip3p.offxml` (#1188)

    * Add Electrostatics section to data/test_forcefields/tip3p.offxml

    * Update release history

commit 0c42148
Author: Jeff Wagner <jwagnerjpl@gmail.com>
Date:   Fri Mar 11 13:43:59 2022 -0800

    fix #837 (#1214)

commit 0cb3a72
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 10 14:34:37 2022 -0600

    Fix formatting in a `.assign_partial_charges` docstring (#1212)

commit b8a5793
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Wed Feb 9 11:58:02 2022 -0600

    Remove old mendeleev import

commit 373262a
Merge: 31c7755 0beef75
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Wed Feb 9 11:38:27 2022 -0600

    Merge remote-tracking branch 'upstream/topology-biopolymer-refactor' into add-topology

commit 31c7755
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Wed Feb 9 11:30:23 2022 -0600

    Update Topology.__add__/__iadd__, add tests, fix annotations

commit fc12de1
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Tue Feb 8 15:33:41 2022 -0600

    Add first implementation of Topoogy.__add__

commit 0321d99
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 12:30:12 2022 -0600

    Remove Atom.element

commit a2e7bf9
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 12:25:03 2022 -0600

    Fix virtual site creation

commit a383a0a
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 11:26:19 2022 -0600

    Fix setting masses while adding OpenMM particles

commit 37e86db
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 10:20:30 2022 -0600

    Fix more typos

commit 6555475
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 09:33:13 2022 -0600

    Fix typos, tests

commit c529ee0
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Wed Jan 26 18:49:04 2022 -0600

    Draft a refactor to openff.units.elements

* Preserve constraints in `Topology.__add__`

* Update release history
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@j-wags j-wags j-wags approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Water LibraryCharges aren't used in output OpenMM systems in Sage
2 participants
@mattwthompson @j-wags