-
Notifications
You must be signed in to change notification settings - Fork 89
Introduce the concept of ranking metrics #3721
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3721 +/- ##
=======================================
+ Coverage 99.7% 99.7% +0.1%
=======================================
Files 339 339
Lines 34819 34845 +26
=======================================
+ Hits 34688 34714 +26
Misses 131 131
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
docs/source/demos/text_input.ipynb
Outdated
@@ -453,7 +453,7 @@ | |||
], | |||
"metadata": { | |||
"kernelspec": { | |||
"display_name": "Python 3", | |||
"display_name": "Python 3 (ipykernel)", |
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.
Might need a make lint-fix
?
obj() | ||
for obj in all_objectives_dict.values() | ||
if obj.is_defined_for_problem_type(problem_type) | ||
and (obj not in get_non_core_objectives() or obj in ranking_only_objectives()) |
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.
remind me what's behind the obj not in get_non_core_objectives()
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.
There are some objectives in that list that require additional setup/configuration before they can be used during optimization. An example, the cost benefit matrix needs weights for each true positive/negative and false positive/negative
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.
LGTM - great work!
Returns: | ||
List of ObjectiveBase classes | ||
""" | ||
return [ |
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.
thoughts on adding ranking_only
as a property instead of maintaining a list? I know this is what we do for get_non_core_objectives
as well but maybe we could file this as a future enhancement.
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'm not quite sure I understand your suggestion - do you mean a property of the objective classes themselves? I do actually like that better, I think.
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.
Quick investigation shows that making this switch is unfortunately nontrivial, so I think I'll leave it as is for now. It's a good future enhancement though!
Closes #3710
Note: with this implementation,
get_optimization_objectives
is a roundabout way to get our current core metrics. We will want to move away from core for optimization purposes, but we can’t remove the non-optimization non-core (i.e. recall) metrics until our downstream users have made the switch.