Skip to content

Solvate with OpenMM - #131

Closed
mattwthompson wants to merge 8 commits into
OpenFreeEnergy:mainfrom
mattwthompson:openmm-solvation
Closed

mattwthompson wants to merge 8 commits into
OpenFreeEnergy:mainfrom
mattwthompson:openmm-solvation

Conversation

@mattwthompson

Copy link
Copy Markdown
Collaborator

No description provided.

@codecov

codecov Bot commented Aug 14, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 23.91304% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.53%. Comparing base (5fce839) to head (7c3a7a8).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/pontibus/utils/system_solvation.py 23.91% 35 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #131      +/-   ##
==========================================
- Coverage   95.46%   94.53%   -0.93%     
==========================================
  Files          34       34              
  Lines        2732     2801      +69     
==========================================
+ Hits         2608     2648      +40     
- Misses        124      153      +29     

☔ 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.

@mattwthompson mattwthompson changed the title Initial implementation of OpenMM-based solvation Solvate with OpenMM Sep 12, 2025
Comment on lines +60 to +62
)
from pontibus.utils.system_creation import (
interchange_creation_via_openmm as interchange_packmol_creation,

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is the only place I can find interchange_packmol_creation imported, so I think that this alias forces the OpenMM code path to be used everywhere in tests

$ grep -r interchange_creation_via_openmm src
src/pontibus/utils/system_creation.py:def interchange_creation_via_openmm(
src/pontibus/protocols/relative/hybridtop_units.py:    interchange_creation_via_openmm as interchange_packmol_creation,

@mattwthompson

Copy link
Copy Markdown
Collaborator Author

Worked around this one

FAILED src/pontibus/tests/protocols/relative/test_protocol.py::test_dry_run_ligand - assert 2326 == 2278
 +  where 2326 = len({12: 0, 13: 1, 14: 2, 15: 3, ...})
 +    where {12: 0, 13: 1, 14: 2, 15: 3, ...} = <openfe.protocols.openmm_rfe._rfe_utils.relative.HybridTopologyFactory object at 0x7f7873725d10>._env_old_to_new_map
 +      where <openfe.protocols.openmm_rfe._rfe_utils.relative.HybridTopologyFactory object at 0x7f7873725d10> = <instance of HybridRepexSampler>._hybrid_factory

but not sure what to think of

FAILED src/pontibus/tests/utils/test_interchange_packmol.py::test_split_combine_parameters - AssertionError: assert '[#6X4:1]_DUPLICATE' in ['[#1:1]', '[#1:1]-[#6X4]', '[#1:1]-[#6X4]-[#7,#8,#9,#16,#17,#35]', '[#1:1]-[#6X4](-[#7,#8,#9,#16,#17,#35])-[#7,#8,#9,#16,#17,#35]', '[#1:1]-[#6X4](-[#7,#8,#9,#16,#17,#35])(-[#7,#8,#9,#16,#17,#35])-[#7,#8,#9,#16,#17,#35]', '[#1:1]-[#6X4]~[*+1,*+2]', ...]
 +  where '[#6X4:1]_DUPLICATE' = PotentialKey associated with handler 'vdW' with id '[#6X4:1]_DUPLICATE'.id

@mattwthompson

Copy link
Copy Markdown
Collaborator Author

This is far from a complete, polished work but I think it's in a state which would benefit from some feedback (@IAlibay is surely the most knowledgeable)

  • Is the above test failure concerning? It's not part of a code path I intended to change
  • Is this generally the right path to be going down?
  • What's missing before we can try some runs?

@IAlibay
IAlibay self-requested a review September 12, 2025 16:49
@IAlibay

IAlibay commented Sep 12, 2025

Copy link
Copy Markdown
Member

This is far from a complete, polished work but I think it's in a state which would benefit from some feedback (@IAlibay is surely the most knowledgeable)

  • Is the above test failure concerning? It's not part of a code path I intended to change
  • Is this generally the right path to be going down?
  • What's missing before we can try some runs?

Thanks, I'll try to review next week.

@IAlibay

IAlibay commented Sep 12, 2025

Copy link
Copy Markdown
Member

@mattwthompson there's a few things that would be faster done if I can PR into this PR. How much of a pain would it be to move this branch from a fork to this repo directly?

@mattwthompson mattwthompson mentioned this pull request Sep 12, 2025
@mattwthompson

Copy link
Copy Markdown
Collaborator Author

#140

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