search

OpenFreeEnergy / kartograf

This package contains tools for setting up hybrid-topology FE calculations
https://kartograf.readthedocs.io/
MIT License
25 stars 3 forks source link

Fix ring hybridization filter #65

Closed jthorton closed 1 week ago

jthorton commented 1 week ago

Fixes #62.

codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.60%. Comparing base (ca54878) to head (f670838). Report is 4 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #65 +/- ## ======================================= Coverage 96.60% 96.60% ======================================= Files 13 13 Lines 618 618 ======================================= Hits 597 597 Misses 21 21 ```

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

jthorton commented 1 week ago

@hannahbaumann great point, I think the atomwise hybridization filter should handle those cases currently but it would be good to extend this filter also to do the same. I believe that using the atom hybridization and map_exact_ring_matches_only=True should do the same filtering as this filter on rings but handle non-ring cases as well so for users that only want to handle ring cases, we should make the behaviour consistent. I'll open another issue for that and look to do it in another PR though to keep this one simple.

hannahbaumann commented 1 week ago

Thanks @jthorton , that sounds great!