Add : **predict_params in fit and predict method for Mapie Regression #471
Conversation
thibaultcordier
left a comment
thibaultcordier
left a comment
There was a problem hiding this comment.
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. |
thibaultcordier
left a comment
thibaultcordier
left a comment
There was a problem hiding this comment.
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
|
Note that for the description of the |
LacombeLouis
left a comment
LacombeLouis
left a comment
There was a problem hiding this comment.
Good! One last change and we're there!
thibaultcordier
left a comment
thibaultcordier
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
LacombeLouis
left a comment
LacombeLouis
left a comment
There was a problem hiding this comment.
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!
BaptisteCalot
deleted the
212-predict_params_mapie_without_classification
branch
LacombeLouis
changed the title
4 participants
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 lintmake type-checkmake testsmake coveragemake doc