Skip to content

Conversation

@Immi000
Copy link
Contributor

@Immi000 Immi000 commented Nov 26, 2025

This adds the missing lambda-factor referred to here.

@codecov
Copy link

codecov bot commented Nov 26, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.62%. Comparing base (0bd1d62) to head (3ea323b).
⚠️ Report is 6 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #232   +/-   ##
=======================================
  Coverage   68.61%   68.62%           
=======================================
  Files          46       46           
  Lines        9120     9121    +1     
=======================================
+ Hits         6258     6259    +1     
  Misses       2862     2862           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jgreener64
Copy link
Member

Thanks for this. Would you mind updating the docstring and making the analogous changes for CoulombSoftCoreBeutler too?

I guess the Gapsys soft core variants should also have this change?

@Immi000 Immi000 marked this pull request as draft November 26, 2025 14:26
@Immi000
Copy link
Contributor Author

Immi000 commented Nov 26, 2025

I added the missing λ to CoulombSoftCoreBeutler as well and updated the CoulombSoftCoreBeutler and LennardJonesSoftCoreBeutler docstrings correspondingly.

As far as I can tell the Gapsys formulation depends on λ only implicitly via the switching radius which seems to be implemented correctly, so I think no adjustment is necessary there.

@Immi000 Immi000 marked this pull request as ready for review November 26, 2025 14:52
@jgreener64
Copy link
Member

Thanks for the changes. Having a look it seems that OpenFE also applies the same λ weighting to the Gapsys variants (https://github.com/OpenFreeEnergy/openfe/blob/013b4b3213b8329339607296d371df8b8e207b92/openfe/protocols/openmm_rfe/_rfe_utils/relative.py#L810), so I think those should change as well.

@Immi000
Copy link
Contributor Author

Immi000 commented Nov 29, 2025

Right, that makes sense, I seem to have looked at the single state formula instead of the full interpolation. I've adjusted the Gapsys LJ and Coulomb force and potential.

@jgreener64 jgreener64 merged commit 9451266 into JuliaMolSim:master Nov 29, 2025
7 of 9 checks passed
@jgreener64
Copy link
Member

Great, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants