Skip to content

Septop move files#1932

Open
IAlibay wants to merge 10 commits intomainfrom
septop_move_files
Open

Septop move files#1932
IAlibay wants to merge 10 commits intomainfrom
septop_move_files

Conversation

@IAlibay
Copy link
Copy Markdown
Member

@IAlibay IAlibay commented Apr 8, 2026

Fixes #1918

Move SepTop unit and results files to separate files, along with minor QoL improvements.

Checklist

  • All new code is appropriately documented (user-facing code must have complete docstrings).
  • Added a news entry, or the changes are not user-facing.
  • Ran pre-commit: you can run pre-commit locally or comment on this PR with pre-commit.ci autofix.

Manual Tests: these are slow so don't need to be run every commit, only before merging and when relevant changes are made (generally at reviewer-discretion).

Developers certificate of origin

@IAlibay
Copy link
Copy Markdown
Member Author

IAlibay commented Apr 8, 2026

pre-commit.ci autofix

@IAlibay IAlibay requested a review from hannahbaumann April 8, 2026 19:25
@IAlibay IAlibay marked this pull request as ready for review April 8, 2026 19:25
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 8, 2026

Codecov Report

❌ Patch coverage is 96.19289% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.35%. Comparing base (c60db06) to head (b435d8d).

Files with missing lines Patch % Lines
...protocols/openmm_septop/septop_protocol_results.py 90.62% 12 Missing ⚠️
src/openfe/protocols/openmm_septop/septop_units.py 98.85% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1932      +/-   ##
==========================================
- Coverage   94.62%   91.35%   -3.28%     
==========================================
  Files         206      208       +2     
  Lines       18285    18306      +21     
==========================================
- Hits        17303    16723     -580     
- Misses        982     1583     +601     
Flag Coverage Δ
fast-tests 91.35% <96.19%> (?)
slow-tests ?

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

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

Copy link
Copy Markdown
Contributor

@hannahbaumann hannahbaumann left a comment

Choose a reason for hiding this comment

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

Thanks @IAlibay , this looks good to me! I'm not sure if I saw all the QoL changes you made to the results or units files, so if there's anything particular you would like me to look at, please let me know.
This may make merging into the membrane PR a bit tricky, but that's also not a big problem.

@IAlibay
Copy link
Copy Markdown
Member Author

IAlibay commented Apr 9, 2026

This may make merging into the membrane PR a bit tricky, but that's also not a big problem.

That's a good point, let's aim to merge the membrane PR first. I can deal with the merge conflict here after the fact.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 9, 2026

No API break detected ✅

@atravitz atravitz added this to the 1.11.0 milestone Apr 9, 2026
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.

Move units, settings and protocol to separate files for SepTop

3 participants