-
Notifications
You must be signed in to change notification settings - Fork 9
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
Make Transformation
and NonTransformation
subclass TransformationBase
#311
Conversation
…nBase` In some cases it can be awkward for `NonTransformation` to be a subclass of `Transformation`, such as in `alchemiscale`, for cases where `NonTransformation` should be handled very differently. Switching to a shared, abstract base class for `Transformation` and `NonTransformation` simplifies this.
Hello @dotsdl! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-04-25 06:03:17 UTC |
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.
Thanks for opening this @dotsdl - as a first initial triage, could you possibly add in a rever rst file with a short summary of this update and any expected API breaks?
I can do that, no problem @IAlibay! I also need to fix the broken tests yet as well. |
To be clear, this isn't blocking anything on |
@dotsdl possibly a crazy question - does this technically change the gufe keys for transformation objects? I can't remember if it captures the parent class signatures (partly asking because of the test failures) |
The follow up question I have is if we need a 1.x gufe policy on key stability? |
Will pursue a review on this from Alyssa when she starts. |
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.
This looks good to me, couple of comments though.
Addressed conflicts and refined docstrings, addressed reviews.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #311 +/- ##
==========================================
- Coverage 98.67% 98.49% -0.18%
==========================================
Files 36 36
Lines 2117 2131 +14
==========================================
+ Hits 2089 2099 +10
- Misses 28 32 +4 ☔ View full report in Codecov by Sentry. |
@IAlibay can I get another review from you? I also used this as an opportunity to clean up some docstrings, and adjust the reprs for |
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.
lgtm thanks!
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.
This refactor is a big improvement imo, just a couple docstrings and clarifying questions.
Co-authored-by: Alyssa Travitz <[email protected]>
Co-authored-by: Alyssa Travitz <[email protected]>
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
In some cases it can be awkward for
NonTransformation
to be a subclass ofTransformation
, such as inalchemiscale
, for cases whereNonTransformation
should be handled very differently.Switching to a shared, abstract base class for
Transformation
andNonTransformation
simplifies this.