Skip to content

Conversation

@hannahbaumann
Copy link
Contributor

As described in this issue (#413) SystemGenerator currently only supports the MonteCarloBarostat.
Here, instead of re-creating the barostat in _modify_forces, the barostat is copied, however, there may be reasons why it was necessary to re-create the barostat?

@hannahbaumann hannahbaumann changed the title Allow for any barostat in SystemGenerator [ WIP ] Allow for any barostat in SystemGenerator Oct 27, 2025
@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.17%. Comparing base (839f225) to head (4d11936).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #414      +/-   ##
==========================================
+ Coverage   84.14%   84.17%   +0.02%     
==========================================
  Files           5        5              
  Lines         776      771       -5     
==========================================
- Hits          653      649       -4     
+ Misses        123      122       -1     

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

@epretti
Copy link
Member

epretti commented Oct 30, 2025

Sorry this was sitting here for a bit; I forgot someone needed to press a button to actually make the CI run. This looks good to me provided the tests are passing. Is it ready or still WIP?

@mattwthompson
Copy link
Collaborator

I think we should have at least one test: one that takes in a barostat (ideally of a different type than the default) and make sure that it comes out as expected

@epretti
Copy link
Member

epretti commented Oct 30, 2025

Good point; I saw we already have a test here

but it only tests the standard MonteCarloBarostat type. Any type of barostat should now work but it'd probably be good to have the test cover one of the other types.

@hannahbaumann
Copy link
Contributor Author

Thanks! I added the MonteCarloMembraneBarostat to the test!

@hannahbaumann hannahbaumann changed the title [ WIP ] Allow for any barostat in SystemGenerator Allow for any barostat in SystemGenerator Oct 31, 2025
@epretti
Copy link
Member

epretti commented Nov 3, 2025

Looks alright to me, although I haven't worked a lot on SystemGenerator so I might be missing something: @mattwthompson anything else that stands out to you that should be addressed?

@mattwthompson
Copy link
Collaborator

I don't use SystemGenerator but this does basically what I expected and tests the appropriate behavior. I would be surprised if this breaks something, but if it does that would indicate something isn't tested and we can patch that later

@epretti epretti merged commit a81f28a into openmm:main Nov 4, 2025
14 checks passed
@mikemhenry mikemhenry mentioned this pull request Dec 16, 2025
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.

4 participants