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

Test expected failing or unhandled cases #96

Merged
merged 13 commits into from
Nov 7, 2024

Conversation

ijpulidos
Copy link
Contributor

This set of changes is aimed at writing test cases that are expected to fail in the protein mutation protocol, either because of limitations of the methodology or because these are simply cases that we don't support to date.

Solves #80

@ijpulidos ijpulidos linked an issue Oct 31, 2024 that may be closed by this pull request
@ijpulidos ijpulidos marked this pull request as ready for review October 31, 2024 13:44
@ijpulidos
Copy link
Contributor Author

As usual, just bear in mind that these tests will fail as of today. We will be implementing the protocol to comply to these tests eventually. Some tweaks might be needed when that happens, but the idea is that the overall shape of these tests stay the same when we implement the protein mutation protocol.

Copy link
Collaborator

@jthorton jthorton left a comment

Choose a reason for hiding this comment

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

The tests make sense and seem to match the descriptions.

Some minor comments which are not blocking and for my own understanding:

  • is there a reason generally why we want to avoid ring growing or double charge changes it seems like these transformations while more difficult should be possible.
  • It would be good to have a way to find these error transformations ahead of run time in general, consider the case of using alchemiscale you won't know that some of these will fail till submission where as it might be nice to have some protocol validation when constructing an AlchemicalNetwork or at some stage earlier in the pipeline.

@IAlibay
Copy link
Member

IAlibay commented Nov 4, 2024

is there a reason generally why we want to avoid

ring growing

Handling ring breaking is something that the current hybrid topology cannot handle properly - it's a fundamental issue in the way in which we deal with bond scaling. There are some ways around this, but they would require a rather large rewrite of the hybrid topology approach and generally isn't something we want to invest a lot of our time on.

double charge changes

This is related to OpenFreeEnergy/openfe#660

The main answer here is "the tooling doesn't allow it now, we could do it, but it's not the best use of our resources".

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.

I'm not super convinced on the error names, might be something we need to workshop in the longer run.

feflow/utils/exceptions.py Outdated Show resolved Hide resolved
"""Module with custom exceptions that can be useful for including in protocols"""


class ProtocolError(Exception):
Copy link
Member

Choose a reason for hiding this comment

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

If you want to create a base error type, this really should be in gufe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes a lot of sense, I opened a ticket for it OpenFreeEnergy/gufe#385

feflow/utils/exceptions.py Outdated Show resolved Hide resolved
@ijpulidos
Copy link
Contributor Author

I renamed some of the custom errors and rewrote some of the strings, I think we could probably reiterate on these if needed in the future. I'll go ahead and merge this.

@ijpulidos ijpulidos merged commit 00c1f15 into protein-mutation-protocol Nov 7, 2024
1 of 9 checks passed
@ijpulidos ijpulidos deleted the tests/expected-fails branch November 7, 2024 21:24
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.

Write some negative control tests (expected failures)
3 participants