-
Notifications
You must be signed in to change notification settings - Fork 128
Add : **predict_params in fit and predict method for Mapie Classifier #502
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
Add : **predict_params in fit and predict method for Mapie Classifier #502
Conversation
LacombeLouis
left a comment
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.
Hey @BaptisteCalot, thank you for this PR! Just one interrogation that is repeated multiple times.
thibaultcordier
left a comment
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.
Thank you for your contributions !
I suggest you replace ValueError with UserWarning.
| with pytest.raises(ValueError, match=( | ||
| r"Using one 'predict_param' in the fit method " | ||
| r"without using one 'predict_param' in the predict method. " | ||
| r"Please ensure a similar configuration of 'predict_param' " | ||
| r"is used in the predict method as called in the fit." | ||
| )): | ||
| mapie_fitted.predict(X_test) |
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.
Test if a UserWarning is raised if better.
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.
Hey @thibaultcordier, why do you want to raise a warning. We believed an error was more appropriate, since if we don't use predict_params in both, it's clearly an issue.
Description
Adding **predict_params for the predict methods of the models
Fixes #491
Type of change
Please remove options that are irrelevant.
How Has This Been Tested?
Checklist
make lintmake type-checkmake testsmake coveragemake doc