-
Notifications
You must be signed in to change notification settings - Fork 3
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 optional filter ordering #64
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #64 +/- ##
=======================================
Coverage 96.60% 96.60%
=======================================
Files 13 13
Lines 618 618
=======================================
Hits 597 597
Misses 21 21 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jthorton , lgtm, just that one question!
mapper = KartografAtomMapper( | ||
map_exact_ring_matches_only=True, | ||
additional_mapping_filter_functions=[filter_hybridization_changes] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would happen if you include the default filters in the additional mapping filters, would it skip them/still preserve the correct order?
E.g.
additional_mapping_filter_functions=[
filter_ringbreak_changes, # default
filter_ringsize_changes, # default
filter_whole_rings_only, # default
filter_hybridization_changes]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently don't check what filters are passed as additional so if a default filter was used then it would be applied twice first as an additional filter then as the default, maybe I should expand the docs to make it clear that defaults are applied first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the behaviour we want to have?
Long term I could see a case where folks would want to chain custom filters so that they can iteratively patch some behaviour - however in this case where we have named filters vs custom ones, would it make sense to just error out (or at least warn) if the method is already in _filter_funcs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a warning would be fair we don't want to error out and stop experimentation and a section in the docs which explains exactly which filters are applied when you set a default filter should help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall lgtm, but would be good to work out what we want our default behaviour to be.
mapper = KartografAtomMapper( | ||
map_exact_ring_matches_only=True, | ||
additional_mapping_filter_functions=[filter_hybridization_changes] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the behaviour we want to have?
Long term I could see a case where folks would want to chain custom filters so that they can iteratively patch some behaviour - however in this case where we have named filters vs custom ones, would it make sense to just error out (or at least warn) if the method is already in _filter_funcs
?
Fixes #61 by applying additional filters before the default ring-based filters to ensure rings are not broken when requested.