add scale14 to global params

[pavankum]

Description

To resolve #36

Status

• Ready to go

[codecov-commenter]

Codecov Report

Merging #37 (02447df) into main (b7d8856) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #37   +/-   ##
=======================================
  Coverage   78.62%   78.62%           
=======================================
  Files           5        5           
  Lines         276      276           
=======================================
  Hits          217      217           
  Misses         59       59           

Flag	Coverage Δ
unittests	78.62% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
smirnoff_plugins/handlers/nonbonded.py	94.26% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[mattwthompson]

This seems good to me - based on how I understand the code to work and you reporting that it works in the linked issue.

In principle there should be tests, etc., but seeing as

• this is more of an experimental repo than a production-facing one
• this has already been tested in some capacity (all the way to getting results!)
• branches diverging is undesirable
• I see a 👍 from @jthorton

I think we should merge this as-is - and we should pull the trigger in a day or two if no more comments.

[jthorton]

@pavankum thanks for looking into this, I was a little lost as to why this fixed the issue as the scale14 parameter is already added to the global parameter list explicitly in the creation of the custom bond force which holds the 1-4 interactions here. It seems like the root cause of this is in forcebalance when it creates a copy of the system to evaluate the perturbed parameters it only looks for global values in the CustomNonbondedForce but if it also checked the CustomBondForce it would copy both.

[jthorton]

I think it's probably easier to fix this here but I have suggested the fix upstream to forcebalance here, feel free to do the fits using this branch @pavankum as it will take a while to get a new forcebalance version.

[pavankum]

Ahh okay, thank you @jthorton for fixing it in FB. Thank you for the review @mattwthompson, since this is fixed in FB I think we need not merge this in master.

[mattwthompson]

Do these patches conflict with each other? All things being equal I'd rather have it here since we have more control over the release cycle and it's easier to pin (well, add a lower constraint) this package than ForceBalance.

[pavankum]

Yeah, I think we would be double counting the contributions if both patches are applied. I can still work from this branch and it's not a blocker for me. I would leave it to you and @jthorton to decide what's the best way forward.

[jthorton]

I agree it would be easier to fix here and this will fix our pathway of using forcebalance with systems made via the toolkit but will leave it broken for anyone using custom forces via OpenMM xmls, although I don't think that is very common at all! So feel free to merge and Ill leave my FB PR for now and see what happens.

[mattwthompson]

Sounds good to me. Let me know if you want this in a release (0.0.3), otherwise I assume you and @pavankum can keep running experiments from the main branch as useful