-
-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
ENH Add routing to LogisticRegressionCV #26525
ENH Add routing to LogisticRegressionCV #26525
Conversation
re:tests: yes, they were quite nice, it'd be nice to have them here. |
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.
This is great, I'd also add a changelog to 1.4.
Should this be a feature? |
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.
Thanks @OmarManzoor this is great!
@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
if 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be useful to get the link to the user guide.
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 mean this page: https://scikit-learn.org/stable/metadata_routing.html
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here regarding an informative docstring.
@pytest.mark.parametrize( | ||
"cv_scorer", | ||
CV_SCORERS, | ||
) |
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
LGTM. Thanks @OmarManzoor
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?