-
Notifications
You must be signed in to change notification settings - Fork 1
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
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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. |
There was a problem hiding this 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.
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.
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". |
There was a problem hiding this 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.
"""Module with custom exceptions that can be useful for including in protocols""" | ||
|
||
|
||
class ProtocolError(Exception): |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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. |
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