Enable `PersesAtomMapper` #929

jthorton · 2024-09-03T15:30:21Z

Add the missing abstract methods for the PersesAtomMapper and turn on the tests.
Checklist

Added a news entry

Developers certificate of origin

I certify that this contribution is covered by the MIT License here and the Developer Certificate of Origin at https://developercertificate.org/.

pep8speaks · 2024-09-03T15:30:27Z

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-09-10 14:00:03 UTC

RiesBen

I just have minor comments, appart from those looks good to me :)

openfe/tests/setup/atom_mapping/test_perses_atommapper.py

codecov · 2024-09-05T02:26:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.85%. Comparing base (733908d) to head (79de2e8).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #929      +/-   ##
==========================================
- Coverage   94.32%   92.85%   -1.48%     
==========================================
  Files         134      134              
  Lines        9917     9932      +15     
==========================================
- Hits         9354     9222     -132     
- Misses        563      710     +147

Flag	Coverage Δ
fast-tests	`92.85% <100.00%> (?)`
slow-tests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

IAlibay

Just the one thing from me

openfe/setup/atom_mapping/perses_mapper.py

jthorton · 2024-09-05T10:19:33Z

I noticed that the type hint for coordinate_tolerance was float which I have updated, I am not sure if this will cause issues in some JSON schema files. Are the atom mapper settings ever saved to file and should I just leave this as float for now and look at this in a PR later?

IAlibay · 2024-09-05T11:01:34Z

I noticed that the type hint for coordinate_tolerance was float which I have updated, I am not sure if this will cause issues in some JSON schema files. Are the atom mapper settings ever saved to file and should I just leave this as float for now and look at this in a PR later?

Sorry I don't fully understand what you mean here, could you elaborate? As far as I know we haven't ever gotten around to serializing AtomMappers (@RiesBen please correct me!).

Regarding the float -> unit.Quantity, this looks like a legitimate bug being fixed, i.e. if you passed a float it should have been failed, since AtomMapper expects a quantity. I'm happy going with this unless your JSON question highlights something I've completely unware/forgotten about.

IAlibay

Looks good to me thanks!

openfe/setup/atom_mapping/perses_mapper.py

RiesBen · 2024-09-05T11:13:06Z

I noticed that the type hint for coordinate_tolerance was float which I have updated, I am not sure if this will cause issues in some JSON schema files. Are the atom mapper settings ever saved to file and should I just leave this as float for now and look at this in a PR later?

Sorry I don't fully understand what you mean here, could you elaborate? As far as I know we haven't ever gotten around to serializing AtomMappers (@RiesBen please correct me!).

Regarding the float -> unit.Quantity, this looks like a legitimate bug being fixed, i.e. if you passed a float it should have been failed, since AtomMapper expects a quantity. I'm happy going with this unless your JSON question highlights something I've completely unware/forgotten about.

oooooooh wait, we do serialize atom mappers!
This is actually very important, that they are serializable, as Konnektor will bring parallelization of Network generation (and mappings) . If they are not serializable, you will not be able to go parallel. :)

jthorton · 2024-09-05T11:27:50Z

If they are not serializable, you will not be able to go parallel. :)

We might be getting two different cases confused, Konnector needs them to work with pickle right for the processpool? which quantities should work with. I meant JSON serialisation as I am unsure it will know what to do with a quantity but maybe that's what the to_dict method is for to work around this?

IAlibay · 2024-09-05T19:56:24Z

If they are not serializable, you will not be able to go parallel. :)

We might be getting two different cases confused, Konnector needs them to work with pickle right for the processpool? which quantities should work with. I meant JSON serialisation as I am unsure it will know what to do with a quantity but maybe that's what the to_dict method is for to work around this?

Ah ok yes, if your concern is "will X in a gufe serializable go to JSON", in openfe/gufe we use custom codecs to do this: https://github.com/OpenFreeEnergy/gufe/blob/main/gufe/custom_codecs.py

We do not have a specific codec for openmm units/quantities. In this case, the answer is to switch over to openff-units.

IAlibay · 2024-09-05T19:57:43Z

This is to say @jthorton we should just do the switch to openff units now. It's an API break but I'm happy to label this as a bugfix.

IAlibay · 2024-09-05T19:59:09Z

openfe/setup/atom_mapping/perses_mapper.py

@@ -43,7 +67,7 @@ def __init__(self, allow_ring_breaking: bool = True,
            if mappings must strictly preserve chirality, default: True
        use_positions: bool, optional
            this option defines, if the
-        coordinate_tolerance: float, optional
+        coordinate_tolerance: openmm.unit.Quantity, optional


A note, we say "optional" but none of the type hints say Optional[...].. maybe that's something we should also remember to fix.

IAlibay

Let's switch to openff units and add a news entry to make it clear we've changed the behaviour (as a bug fix).

RiesBen

This one looks cool to me, after @IAlibay comments done and he agrees. :)
Cool addition @jthorton :)

jthorton · 2024-09-10T14:12:45Z

Thanks @RiesBen and @IAlibay for the review changes done in 79de2e8. The perses tests are still not running though as most depend on openeye shall I just try and include the dict round trip one which shouldn't need openeye?

IAlibay · 2024-09-10T14:29:56Z

We should have an openeye runner - will need to look into what's happening

jthorton · 2024-09-10T14:37:17Z

It looks like openeye is not enabled in the testing CI from what I can follow here.

mikemhenry · 2024-09-10T21:25:15Z

The openeye tests are not running since this is from a fork, which won't have access to the OE license we have for CI, we can either give @jthorton write access to our repo OR I can push this branch to the repo which will then allow the tests to run.

jthorton · 2024-09-11T07:29:25Z

Thanks @mikemhenry either is fine for me!

IAlibay · 2024-09-11T11:50:23Z

Let's give him write access.

mikemhenry · 2024-09-11T20:40:50Z

@jthorton you should have write access, go here: https://github.com/OpenFreeEnergy/openfe to accept the invite if it doesn't show up. Then you should be able to push this branch directly to our repository.

add abstract methods for PersesAtomMApper, activate tests

b2cdc30

jthorton mentioned this pull request Sep 3, 2024

[DNM] OpenFE 1.0 + Alchemiscale 0.5 compat asapdiscovery/asapdiscovery#1170

Merged

1 task

dotsdl requested a review from RiesBen September 4, 2024 05:29

lint

075e07e

RiesBen requested changes Sep 4, 2024

View reviewed changes

openfe/tests/setup/atom_mapping/test_perses_atommapper.py Outdated Show resolved Hide resolved

openfe/tests/setup/atom_mapping/test_perses_atommapper.py Outdated Show resolved Hide resolved

openfe/tests/setup/atom_mapping/test_perses_atommapper.py Outdated Show resolved Hide resolved

IAlibay requested changes Sep 5, 2024

View reviewed changes

openfe/setup/atom_mapping/perses_mapper.py Show resolved Hide resolved

openfe/setup/atom_mapping/perses_mapper.py Outdated Show resolved Hide resolved

update unit type hints, add dict round trip test

eba4f71

IAlibay approved these changes Sep 5, 2024

View reviewed changes

openfe/setup/atom_mapping/perses_mapper.py Show resolved Hide resolved

update doc string

3fe9d43

IAlibay reviewed Sep 5, 2024

View reviewed changes

IAlibay requested changes Sep 5, 2024

View reviewed changes

RiesBen self-requested a review September 7, 2024 21:39

RiesBen approved these changes Sep 7, 2024

View reviewed changes

update perses atom mapper to use openff units

79de2e8

jthorton mentioned this pull request Sep 12, 2024

Perses mapper #930

Merged

2 tasks

jthorton closed this Sep 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable `PersesAtomMapper` #929

Enable `PersesAtomMapper` #929

jthorton commented Sep 3, 2024 •

edited

Loading

pep8speaks commented Sep 3, 2024 •

edited

Loading

RiesBen left a comment

codecov bot commented Sep 5, 2024 •

edited

Loading

IAlibay left a comment

jthorton commented Sep 5, 2024 •

edited

Loading

IAlibay commented Sep 5, 2024

IAlibay left a comment

RiesBen commented Sep 5, 2024 •

edited

Loading

jthorton commented Sep 5, 2024 •

edited

Loading

IAlibay commented Sep 5, 2024

IAlibay commented Sep 5, 2024

IAlibay Sep 5, 2024

IAlibay left a comment

RiesBen left a comment

jthorton commented Sep 10, 2024

IAlibay commented Sep 10, 2024

jthorton commented Sep 10, 2024

mikemhenry commented Sep 10, 2024

jthorton commented Sep 11, 2024

IAlibay commented Sep 11, 2024

mikemhenry commented Sep 11, 2024

Enable PersesAtomMapper #929

Enable PersesAtomMapper #929

Conversation

jthorton commented Sep 3, 2024 • edited Loading

Developers certificate of origin

pep8speaks commented Sep 3, 2024 • edited Loading

Comment last updated at 2024-09-10 14:00:03 UTC

RiesBen left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 5, 2024 • edited Loading

Codecov Report

IAlibay left a comment

Choose a reason for hiding this comment

jthorton commented Sep 5, 2024 • edited Loading

IAlibay commented Sep 5, 2024

IAlibay left a comment

Choose a reason for hiding this comment

RiesBen commented Sep 5, 2024 • edited Loading

jthorton commented Sep 5, 2024 • edited Loading

IAlibay commented Sep 5, 2024

IAlibay commented Sep 5, 2024

IAlibay Sep 5, 2024

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

RiesBen left a comment

Choose a reason for hiding this comment

jthorton commented Sep 10, 2024

IAlibay commented Sep 10, 2024

jthorton commented Sep 10, 2024

mikemhenry commented Sep 10, 2024

jthorton commented Sep 11, 2024

IAlibay commented Sep 11, 2024

mikemhenry commented Sep 11, 2024

Enable `PersesAtomMapper` #929

Enable `PersesAtomMapper` #929

jthorton commented Sep 3, 2024 •

edited

Loading

pep8speaks commented Sep 3, 2024 •

edited

Loading

codecov bot commented Sep 5, 2024 •

edited

Loading

jthorton commented Sep 5, 2024 •

edited

Loading

RiesBen commented Sep 5, 2024 •

edited

Loading

jthorton commented Sep 5, 2024 •

edited

Loading