Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1808 +/- ##
==========================================
- Coverage 94.79% 91.20% -3.60%
==========================================
Files 205 206 +1
Lines 17957 18241 +284
==========================================
- Hits 17022 16636 -386
- Misses 935 1605 +670
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
pre-commit.ci autofix |
|
pre-commit.ci autofix |
| return reporter | ||
|
|
||
| @staticmethod | ||
| def _get_sampler( |
There was a problem hiding this comment.
This method is annoyingly close to the RFE one, but also annoyingly just a bit different.. (because sampler building goes through extra hoops in hybrid topology sims). It might be hard to align them in the future without fixing sampler.create in hybrid topology sims.
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
| "openmm_version": openmm.__version__, | ||
| "openfe_version": openfe.__version__, | ||
| "gufe_version": gufe.__version__, |
There was a problem hiding this comment.
Looking at aligning how the protocols do this, the Htop method does it in the units result dict in run rather then in execute
openfe/src/openfe/protocols/openmm_rfe/hybridtop_units.py
Lines 806 to 808 in d69baa6
There was a problem hiding this comment.
Ah I thought I moved it on htop to also do it in execute!
So my idea here is that execute should do all the "stuff that handles how units interact with each other in a dag", and run should be solely the self-execution of a Unit.
At the end of the day run is just a convenience so that we can execute a unit with whatever data and get the results. It's not really part of the gufe API. So I think it would be great to keep it as agnostic of the DAG it sits in as possible.
| ----- | ||
| For now this just checks if the netcdf files are present in the | ||
| shared directory but in the future this may expand depending on | ||
| how warehouse works. |
There was a problem hiding this comment.
Missing raises from the docstring
| "openmm_version": openmm.__version__, | ||
| "openfe_version": openfe.__version__, | ||
| "gufe_version": gufe.__version__, |
There was a problem hiding this comment.
Ah I should have scrolled down they are aligned, good job!
There was a problem hiding this comment.
Oops, I did the same thing you did!
| def test_check_restart_one_file_missing(protocol_settings, ahfe_vac_trajectory_path): | ||
| protocol_settings.vacuum_output_settings.checkpoint_storage_filename = "foo.nc" | ||
|
|
||
| errmsg = "One of either the trajectory or checkpoint files are missing" |
There was a problem hiding this comment.
After seeing this message here I think it might be more helpful to tell users which one is actually missing?
Co-authored-by: Josh Horton <Josh.Horton@newcastle.ac.uk>
|
No API break detected ✅ |
jthorton
left a comment
jthorton
left a comment
There was a problem hiding this comment.
This is great, I think this is highlighting places where we can deduplicate across protocols in the future as well. Please merge after fixing the missing file error to be more specific.
2 participants
Fixes #1725
Similar to #1774
Checklist
newsentry, or the changes are not user-facing.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