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

Change hydrogen on hydrogen mapping default to True #69

Open jthorton opened 5 days ago

jthorton commented 5 days ago

Following the review of the default filters in #59, this PR changes the default value for map_hydrogens_on_hydrogens_only to True this should have no change on the performance when using the hybrid topology protocol from OpenFE as these mappings were removed silently as they involve bond constraints.

codecov[bot] commented 5 days ago

Codecov Report

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

Project coverage is 98.15%. Comparing base (4532c95) to head (76c4c9c).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #69 +/- ## ========================================== + Coverage 97.53% 98.15% +0.61% ========================================== Files 13 13 Lines 649 649 ========================================== + Hits 633 637 +4 + Misses 16 12 -4 ```

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

jthorton commented 1 day ago

@IAlibay, you mentioned changing the defaults in openfe as well, but openfe exports the mapper again.

IAlibay commented 1 day ago

@jthorton admittedly I've not put a ton of thought into this, so I could be completely off the mark, what I'm thinking of is we could:

a) ensure that the default settings for the mapper in the CLI has our best practice defaults: https://github.com/OpenFreeEnergy/openfe/blob/be3433c031b3c7e5c0444a4ecbbfdc233f82b4de/openfecli/parameters/plan_network_options.py#L127 b) make sure that any documentation (assuming we have any) that directs the use of Kartograf, tells users to use the right filter options.