Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change hydrogen on hydrogen mapping default to True #69

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jthorton
Copy link
Contributor

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.

@jthorton jthorton self-assigned this Nov 22, 2024
Copy link

codecov bot commented Nov 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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     

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

@jthorton
Copy link
Contributor Author

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

@IAlibay
Copy link
Member

IAlibay commented Nov 26, 2024

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

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to put a block on this pending internal discussions - at least I suggest we skip on this for v1.1.0 and target a v1.2.0 (or maybe even just jump straight to a v2.0!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants