openfe icon indicating copy to clipboard operation
openfe copied to clipboard

[WIP] Restraints API

Open IAlibay opened this issue 1 year ago • 22 comments

Checklist

  • [ ] Added a news entry

Developers certificate of origin

IAlibay avatar Dec 09 '24 12:12 IAlibay

🚨 API breaking changes detected! 🚨

github-actions[bot] avatar Dec 09 '24 12:12 github-actions[bot]

Codecov Report

:x: Patch coverage is 98.80869% with 17 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 93.20%. Comparing base (ccf856a) to head (27447b3). :warning: Report is 226 commits behind head on main.

Files with missing lines Patch % Lines
...protocols/restraint_utils/openmm/omm_restraints.py 92.18% 10 Missing :warning:
openfe/protocols/restraint_utils/geometry/utils.py 96.55% 7 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1043      +/-   ##
==========================================
- Coverage   94.63%   93.20%   -1.44%     
==========================================
  Files         143      165      +22     
  Lines       10962    12389    +1427     
==========================================
+ Hits        10374    11547    +1173     
- Misses        588      842     +254     
Flag Coverage Δ
fast-tests 93.20% <98.80%> (?)
slow-tests ?

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Dec 09 '24 13:12 codecov[bot]

🚨 API breaking changes detected! 🚨

github-actions[bot] avatar Dec 09 '24 15:12 github-actions[bot]

🚨 API breaking changes detected! 🚨

github-actions[bot] avatar Dec 09 '24 17:12 github-actions[bot]

🚨 API breaking changes detected! 🚨

github-actions[bot] avatar Dec 11 '24 11:12 github-actions[bot]

🚨 API breaking changes detected! 🚨

github-actions[bot] avatar Dec 12 '24 12:12 github-actions[bot]

🚨 API breaking changes detected! 🚨

github-actions[bot] avatar Dec 12 '24 14:12 github-actions[bot]

🚨 API breaking changes detected! 🚨

github-actions[bot] avatar Dec 12 '24 14:12 github-actions[bot]

🚨 API breaking changes detected! 🚨

github-actions[bot] avatar Dec 12 '24 14:12 github-actions[bot]

🚨 API breaking changes detected! 🚨

github-actions[bot] avatar Dec 12 '24 21:12 github-actions[bot]

🚨 API breaking changes detected! 🚨

github-actions[bot] avatar Dec 12 '24 21:12 github-actions[bot]

🚨 API breaking changes detected! 🚨

github-actions[bot] avatar Dec 12 '24 21:12 github-actions[bot]

🚨 API breaking changes detected! 🚨

github-actions[bot] avatar Dec 12 '24 21:12 github-actions[bot]

🚨 API breaking changes detected! 🚨

github-actions[bot] avatar Dec 12 '24 22:12 github-actions[bot]

🚨 API breaking changes detected! 🚨

github-actions[bot] avatar Dec 12 '24 22:12 github-actions[bot]

🚨 API breaking changes detected! 🚨

github-actions[bot] avatar Dec 12 '24 22:12 github-actions[bot]

🚨 API breaking changes detected! 🚨

github-actions[bot] avatar Dec 13 '24 01:12 github-actions[bot]

Does OpenMM have pbc wrapping issues with restraints, similar to gromacs? I just tested the code on T4-lysozyme and it selected protein atoms quite far from the ligand which would cause problems in Gromacs (if the distance between atoms is close to half the box size). But I'm not sure if OpenMM deals with this maybe better?

hannahbaumann avatar Jan 28 '25 09:01 hannahbaumann

Does OpenMM have pbc wrapping issues with restraints, similar to gromacs? I just tested the code on T4-lysozyme and it selected protein atoms quite far from the ligand which would cause problems in Gromacs (if the distance between atoms is close to half the box size). But I'm not sure if OpenMM deals with this maybe better?

I'll check re: OpenMM. Regarding "residues picked far away", this looks to me like a consequence of the algorithm design. If we want to pick from nearby residues, we should tighten up the minimum distance criteria and sort the host atoms by their distance from the ligand.

The latter is what I do elsewhere and would be my recommendation, but I didn't want to mess with the implementation too much.

IAlibay avatar Jan 28 '25 10:01 IAlibay

Yes, sorting by distance could make sense! I had this filter (an additional max_filter which I'm not sure if it fully makes sense...) that makes sure that the atoms (different protein atoms relative to each other and to the ligand atom l1), are not further apart than a little less than half the box size. https://github.com/MobleyLab/SeparatedTopologies/blob/e7f712379472ea320cc049a78b19405014d8bbce/boresch_restraints.py#L389-L390

hannahbaumann avatar Jan 28 '25 10:01 hannahbaumann

I had this filter (an additional max_filter which I'm not sure if it fully makes sense...) that makes sure that the atoms (different protein atoms relative to each other and to the ligand atom l1), are not further apart than a little less than half the box size.

Unfortunately filtering by half box isn't as simple when you have a non cubic cell. This is why we instead implemented a "take the minimum combined distance between P3-P1 and P3-P1". Anyways, sorting by distance will yield similar outcomes since we sequentially iterate through the atom list.

IAlibay avatar Jan 28 '25 11:01 IAlibay

No API break detected ✅

github-actions[bot] avatar Jul 04 '25 15:07 github-actions[bot]

Dismissing @hannahbaumann's review as most things have either been addressed or will be addressed in a future PR.

IAlibay avatar Jul 04 '25 15:07 IAlibay