Skip to content

Conversation

@richardjgowers
Copy link
Contributor

Developers certificate of origin

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Just the one thing, otherwise lgtm!


assert est
assert isinstance(est, unit.Quantity)
assert est.is_compatible_with(unit.kilojoule_per_mole)
Copy link
Member

Choose a reason for hiding this comment

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

Could we assert the value for the estimate & uncertainty? (I'm assuming this should remain static unless something goes terribly wrong)

@richardjgowers
Copy link
Contributor Author

the test errors look like this fix here: OpenFreeEnergy/gufe#236

Copy link
Member

@dwhswenson dwhswenson left a comment

Choose a reason for hiding this comment

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

lgtm -- it appears @IAlibay's concern has been addressed?

Only thing I'd suggest changing is something that happened in a previous PR: fixture transformation_json; from the name, I would expect that to be the JSON of a Transformation, not of the results object. Might rename while we're thinking about it?

@richardjgowers richardjgowers force-pushed the rfe_protocolresult_tests branch from df4805c to ab0ae26 Compare September 22, 2023 08:55
@pep8speaks
Copy link

Hello @richardjgowers! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 150:80: E501 line too long (129 > 79 characters)

Line 1258:80: E501 line too long (92 > 79 characters)
Line 1311:43: E231 missing whitespace after ','

@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.28% 🎉

Comparison is base (e8da73d) 91.01% compared to head (ab0ae26) 91.29%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #560      +/-   ##
==========================================
+ Coverage   91.01%   91.29%   +0.28%     
==========================================
  Files         117      117              
  Lines        7220     7287      +67     
==========================================
+ Hits         6571     6653      +82     
+ Misses        649      634      -15     
Files Changed Coverage Δ
openfe/protocols/openmm_rfe/equil_rfe_methods.py 93.04% <ø> (+6.52%) ⬆️
openfe/tests/protocols/conftest.py 60.60% <100.00%> (ø)
...tests/protocols/test_openmm_equil_rfe_protocols.py 90.32% <100.00%> (+1.26%) ⬆️

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

@richardjgowers richardjgowers merged commit f403763 into main Sep 22, 2023
@richardjgowers richardjgowers deleted the rfe_protocolresult_tests branch September 22, 2023 09:56
@atravitz atravitz mentioned this pull request Aug 15, 2025
2 tasks
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.

5 participants