Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force no oechem #479

Merged
merged 3 commits into from
Jul 6, 2023
Merged

Force no oechem #479

merged 3 commits into from
Jul 6, 2023

Conversation

richardjgowers
Copy link
Contributor

No description provided.

@richardjgowers
Copy link
Contributor Author

@IAlibay this is a pain to test inside the protocol but I've got some eg5 edges working with oechem present with this patch.

@IAlibay
Copy link
Contributor

IAlibay commented Jul 5, 2023

I mean this is fine, but I don't understand why we needed to vendor this?

@@ -679,7 +680,9 @@ def run(self, *, dry=False, verbose=True,
def _execute(
self, ctx: gufe.Context, **kwargs,
) -> dict[str, Any]:
outputs = self.run(scratch_basepath=ctx.scratch, shared_basepath=ctx.shared)
with without_oechem_backend():
Copy link
Contributor

@IAlibay IAlibay Jul 5, 2023

Choose a reason for hiding this comment

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

Actually we might need to review this a bit later, I suspect we'll run into trouble when we start offering folks the ability to choose what charge derivation method they can have (for example if they want elf charges)

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Patch coverage: 70.66% and project coverage change: -0.37 ⚠️

Comparison is base (9e6766e) 91.99% compared to head (90787a3) 91.63%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #479      +/-   ##
==========================================
- Coverage   91.99%   91.63%   -0.37%     
==========================================
  Files         106      110       +4     
  Lines        6324     6537     +213     
==========================================
+ Hits         5818     5990     +172     
- Misses        506      547      +41     
Impacted Files Coverage Δ
openfe/utils/handle_trajectories.py 0.00% <0.00%> (ø)
openfecli/tests/commands/test_gather.py 100.00% <ø> (ø)
openfe/tests/protocols/test_openmmutils.py 96.72% <78.57%> (-3.28%) ⬇️
openfecli/commands/gather.py 92.04% <88.46%> (+6.12%) ⬆️
openfe/protocols/openmm_rfe/_rfe_utils/relative.py 83.06% <95.00%> (+1.00%) ⬆️
openfe/protocols/openmm_rfe/equil_rfe_methods.py 93.87% <100.00%> (+4.34%) ⬆️
openfe/protocols/openmm_rfe/equil_rfe_settings.py 86.82% <100.00%> (+5.54%) ⬆️
...tests/protocols/test_openmm_equil_rfe_protocols.py 89.79% <100.00%> (+0.21%) ⬆️
openfe/tests/protocols/test_rfe_tokenization.py 97.14% <100.00%> (ø)
openfe/tests/utils/test_remove_oechem.py 100.00% <100.00%> (ø)
... and 4 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

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

Let's go with this for now, but we'll need to make it optional eventually (probably when it's fixed in openmmforcefields) - that way folks can choose different flavours of charge eventually.

@richardjgowers richardjgowers merged commit 23cb438 into main Jul 6, 2023
5 of 7 checks passed
@richardjgowers richardjgowers deleted the force_no_oechem branch July 6, 2023 07:40
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.

None yet

2 participants