ENH Add routing to LogisticRegressionCV #26525
Conversation
|
re:tests: yes, they were quite nice, it'd be nice to have them here. |
Should this be a feature? |
|
@OmarManzoor there are some merge conflicts which need to be resolved. @thomasjpfan @glemaitre this is ready for a second review. |
|
@OmarManzoor I think the codecov errors are legit and needs testing. |
…ogisticRegressionCV
| i 8000 f params and not _routing_enabled(): | ||
| raise ValueError( | ||
| "params is only supported if enable_metadata_routing=True." | ||
| " See the User Guide for more information." |
There was a problem hiding this comment.
Would be useful to get the link to the user guide.
There was a problem hiding this comment.
I mean this page: https://scikit-learn.org/stable/metadata_routing.html
There was a problem hiding this comment.
I think it will definitely be useful. But if we add it here in one place whereas there are a number of other cases like this (this particular ValueError) that don't contain this link, won't that be a bit inconsistent?
There was a problem hiding this comment.
We can open a subsequent PR to improve the consistency.
| assert pytest.approx(lr_cv1.scores_[1]) != lr_cv2.scores_[1] | ||
|
|
||
|
|
||
| def test_lr_cv_scores_without_enabling_metadata_routing(): |
There was a problem hiding this comment.
Same here regarding an informative docstring.
| @pytest.mark.parametrize( | ||
| "cv_scorer", | ||
| CV_SCORERS, | ||
| ) |
There was a problem hiding this comment.
| @pytest.mark.parametrize( | |
| "cv_scorer", | |
| CV_SCORERS, | |
| ) | |
| @pytest.mark.parametrize("cv_scorer", CV_SCORERS) |
| @pytest.mark.parametrize( | ||
| "cv_splitter", | ||
| CV_SPLITTERS, | ||
| ) |
There was a problem hiding this comment.
| @pytest.mark.parametrize( | |
| "cv_splitter", | |
| CV_SPLITTERS, | |
| ) | |
| @pytest.mark.parametrize("cv_splitter", CV_SPLITTERS) |
| @@ -109,12 +109,18 @@ def record_metadata(obj, method, record_default=True, **kwargs): | |||
| obj._records[method] = kwargs | |||
|
|
|||
|
|
|||
| def check_recorded_metadata(obj, method, **kwargs): | |||
| def check_recorded_metadata(obj, method, split_params=tuple(), **kwargs): | |||
| """Check whether the expected metadata is passed to the object's method.""" | |||
There was a problem hiding this comment.
I think that it will start to be worth documented the parameter here.
For instance what is the meaning of split_params
| @pytest.mark.usefixtures("enable_slep006") | ||
| def test_lr_cv_scores_differ_when_sample_weight_is_requested(): | ||
| """Test sample_weight is correctly passed to the scorer in | ||
| LogisticRegressionCV :meth:`fit` by checking the difference |
There was a problem hiding this comment.
| LogisticRegressionCV :meth:`fit` by checking the difference | |
| `LogisticRegressionCV.fit` by checking the difference |
| @@ -2065,6 +2076,54 @@ def test_liblinear_not_stuck(): | |||
| clf.fit(X_prep, y) | |||
|
|
|||
|
|
|||
| @pytest.mark.usefixtures("enable_slep006") | |||
| def test_lr_cv_scores_differ_when_sample_weight_is_requested(): | |||
| """Test sample_weight is correctly passed to the scorer in | |||
There was a problem hiding this comment.
| """Test sample_weight is correctly passed to the scorer in | |
| """Test `sample_weight` is correctly passed to the scorer in |
| def test_lr_cv_scores_differ_when_sample_weight_is_requested(): | ||
| """Test sample_weight is correctly passed to the scorer in | ||
| LogisticRegressionCV :meth:`fit` by checking the difference | ||
| in scores with the case when sample_weight is not requested. |
There was a problem hiding this comment.
| in scores with the case when sample_weight is not requested. | |
| in scores with the case when `sample_weight` is not requested. |
This comment was marked as outdated.
This comment was marked as outdated.
Co-authored-by: Omar Salman <omar.salman@arbisoft> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
| *, | ||
| pos_class, | ||
| Cs, | ||
| scoring, | ||
| fit_intercept, | ||
| max_iter, | ||
| tol, | ||
| class_weight, | ||
| verbose, | ||
| solver, | ||
| penalty, | ||
| dual, | ||
| intercept_scaling, | ||
| multi_class, | ||
| random_state, | ||
| max_squared_sum, | ||
| sample_weight, | ||
| l1_ratio, | ||
| score_params, |
There was a problem hiding this comment.
With the removal of the default parameters, the docstring would need their removal too.
Co-authored-by: Omar Salman <omar.salman@arbisoft> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Reference Issues/PRs
Closes #25906
Fixes #8950
Follow up of #24498
What does this implement/fix? Explain your changes.
Any other comments?
cc: @adrinjalali
Should I also add the tests that I added for scorers and splitters in test_metaestimators_metadata_routing.py? Also should any additional tests be added now that we have two scenarios, one where the config enable_metadata_routing is True and another where it is False?