-
Notifications
You must be signed in to change notification settings - Fork 111
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 Regression #471
Add : **predict_params in fit and predict method for Mapie Regression #471
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.
Thank you @BaptisteCalot for your contribution to adding predict_params. Just a few suggestions:
- check that your changes in the interface class are compatible with the regression and classification subclasses
- check that your style code is as clear and clean as possible (too many empty lines which make it difficult to read your code)
- use the same convention everywhere (if you don't use a comma at the end of the method signature, do it everywhere)
I question your proposal to add predict-param in initialization. For me, it is not natural or usual to do so.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #471 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 39 44 +5
Lines 4616 5255 +639
Branches 487 890 +403
==========================================
+ Hits 4616 5255 +639 ☔ View full report in Codecov by Sentry. |
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.
Consider previous comments in any part of your changes. The list is non-exhaustive:
- mapie/tests/test_regression.py, line 963: clean the test code by removing the unnecessary empty line
- mapie/tests/test_regression.py, line 1023: clean the test code by removing the unnecessary empty line
- mapie/tests/test_regression.py, line 44: use super init to reduce your code
Co-authored-by: Thibault Cordier <124613154+thibaultcordier@users.noreply.github.com>
Note that for the description of the |
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! One last change and we're there!
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 changes. Just one last one to add docstring into a function.
mapie/tests/test_regression.py
Outdated
|
||
|
||
def test_invalid_predict_parameters() -> None: | ||
"""Test that invalid predict_parameters raise errors.""" |
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.
We're not testing that invalid predict
params give an error. We're checking that if given in predict, we are also given one in fit
, no?
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
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 don't see the solution on this issue. We are checking that if no predict_params
have been given in the fit
, it's an issue.
mapie/tests/test_regression.py
Outdated
|
||
|
||
def test_invalid_predict_parameters() -> None: | ||
"""Test that invalid predict_parameters raise errors.""" |
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 don't see the solution on this issue. We are checking that if no predict_params
have been given in the fit
, it's an issue.
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, just some small updates. Note that you should make a last check for the linting, as the changes I've made don't verify linting!
Description
Adding **predict_params for the predict methods of the models.
Fixes #492
Type of change
Please remove options that are irrelevant
In progress
Checklist
make lint
make type-check
make tests
make coverage
make doc