-
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
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).
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. |
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.
Ok for me.
Nice PR.
Context
Content
This PR includes (lists incrementally updated):