search

OpenFreeEnergy / gufe

grand unified free energy by OpenFE
https://gufe.readthedocs.io
MIT License
28 stars 7 forks source link

Make `Transformation` and `NonTransformation` subclass `TransformationBase` #311

Open dotsdl opened 2 months ago

dotsdl commented 2 months ago

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.

pep8speaks commented 2 months ago

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

Line 75:80: E501 line too long (80 > 79 characters) Line 283:80: E501 line too long (80 > 79 characters)

Comment last updated at 2024-04-25 06:03:17 UTC
dotsdl commented 2 months ago

I can do that, no problem @IAlibay! I also need to fix the broken tests yet as well.

dotsdl commented 2 months ago

To be clear, this isn't blocking anything on alchemiscale; it's just something @ianmkenney and I noticed while working on this one: https://github.com/openforcefield/alchemiscale/pull/270

IAlibay commented 1 week ago

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

IAlibay commented 1 week ago

The follow up question I have is if we need a 1.x gufe policy on key stability?