-
Notifications
You must be signed in to change notification settings - Fork 110
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
Add : **predict_params in fit and predict method for Mapie Classifier #502
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.
Hey @BaptisteCalot, thank you for this PR! Just one interrogation that is repeated multiple times.
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
.
mapie/tests/test_classification.py
Outdated
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 lint
make type-check
make tests
make coverage
make doc