-
Notifications
You must be signed in to change notification settings - Fork 127
Binary risk control - multi risk #762
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
base: master
Are you sure you want to change the base?
Conversation
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.
Good start 💪🏻
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.
Pull Request Overview
This PR adds support for multi-risk control to the BinaryClassificationController by updating type annotations and adding validation logic. It allows users to specify multiple risks and corresponding target levels while maintaining backward compatibility with single risk control.
- Updated type hints to accept lists of risks and target levels alongside single values
- Added validation to ensure risks and target levels lists have matching lengths
- Enhanced class documentation to describe multi-risk control functionality
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
d0b64b6
to
cb272b1
Compare
… and n_obs now always have 2 dimensions (even in mono risk)
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.
Reviewing the __init__
method and submethods.
Nice job!
Regarding typing, we could use a slightly stricter MyPy flag to forbid untyped functions (right now, only partially untyped function are forbidden). But if I remember, using that flag would cause a lot of typing to do in the existing codebase (not 100% sure though). So right now, let's try to not forget typing new code.
cc @GBrelurut
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.
Reviewing method calibrate
(except ltt_procedure
).
|
||
@staticmethod | ||
def _get_risks_and_effective_sample_sizes_per_param( | ||
def _get_risk_values_and_eff_sample_sizes( |
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.
This method is now more generic.
It is not directly unit tested (it is tested undirectly in TestBinaryClassificationControllerSetBestPredictParam
).
We should probably add a multi-risk test case in that class (either in existing test or as a new one).
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.
TestBinaryClassificationControllerSetBestPredictParam tests _set_best_predict_param which only calls this method with one risk (_best_predict_param_choice). The call for multi risk happens in calibrate. So I need to write a test in another class or a new unit test. On it!
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.
done
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
My latest commit updates ltt_procedure for multi risk. Calls to the function and tests have been adapted. Input types and/or shapes have been modified. I also replaced np.where by np.nonzero, which is recommended by numpy doc in the case when only the condition is provided. |
Context
Content
This PR includes (lists incrementally updated):