FEA SLEP006: Metadata routing for SelfTrainingClassifier#28494
Conversation
adrinjalali
left a comment
adrinjalali
left a comment
There was a problem hiding this comment.
A few notes, thanks @adam2392
Signed-off-by: Adam Li <adam2392@gmail.com>
adam2392
left a comment
adam2392
left a comment
There was a problem hiding this comment.
Thank you for the review and pointers! I went thru and fixed the issues in the doc-strings and Bunch.
Signed-off-by: Adam Li <adam2392@gmail.com>
…learn into self-learn-meta
|
Resolved conflicts. Feel free to ping me if there's additional changes desired |
Signed-off-by: Adam Li <adam2392@gmail.com>
|
This PR should not be affected by #28734 |
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
|
Thanks for the review @OmarManzoor! I'll change the last comment if @adrinjalali has any issues (#28494 (comment)) |
OmarManzoor
left a comment
OmarManzoor
left a comment
There was a problem hiding this comment.
LGTM. Thanks @adam2392
| if method_name in ["fit", "partial_fit", "score"]: | ||
| # `fit`, `partial_fit`, 'score' accept y, others don't. | ||
| method(X, y, **method_kwargs) | ||
| except TypeError: | ||
| else: |
There was a problem hiding this comment.
I'm not sure why this makes things hard to debug really. The try/except is more foolproof since a library like imbalance-learn adds some methods to the whole system by patching a few things in sklearn and things would just work.
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
…learn into self-learn-meta
adam2392
left a comment
adam2392
left a comment
There was a problem hiding this comment.
Thanks for the review @adrinjalali ! I addressed your comments in 3b5a3f0
| if method_name in ["fit", "partial_fit", "score"]: | ||
| # `fit`, `partial_fit`, 'score' accept y, others don't. | ||
| method(X, y, **method_kwargs) | ||
| except TypeError: | ||
| else: |
There was a problem hiding this comment.
Fair. I think it was hard before because the error message was not very clear on what method was failing from what class, so I assume that's been fixed with #29226
| if method_name in ["fit", "partial_fit", "score"]: | ||
| # `fit`, `partial_fit`, 'score' accept y, others don't. | ||
| method(X, y, **method_kwargs) | ||
| except TypeError: | ||
| else: |
There was a problem hiding this comment.
I reverted it to the try/except
Signed-off-by: Adam Li <adam2392@gmail.com>
| ) | ||
| else: | ||
| estimator_ = clone(self.estimator) | ||
| return estimator_ |
There was a problem hiding this comment.
still missing the case where both estimator and base_estimator are passed, in which case we need to raise
|
|
||
|
|
||
| # TODO(1.8): remove in 1.8 | ||
| def test_deprecation_warning_base_estimator(): |
There was a problem hiding this comment.
this should also test for all other cases in _get_estimator
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
|
Lmk if I missed anything else @adrinjalali. Thanks for the review and patience! |
adrinjalali
left a comment
adrinjalali
left a comment
There was a problem hiding this comment.
Otherwise LGTM.
I'll let @OmarManzoor have another look since this changed a bit since last he reviewed.
Signed-off-by: Adam Li <adam2392@gmail.com>
|
SG! Thanks for the reviews. |
OmarManzoor
left a comment
OmarManzoor
left a comment
There was a problem hiding this comment.
LGTM. Thanks @adam2392
3 participants
Reference Issues/PRs
Towards: #22893
What does this implement/fix? Explain your changes.
Implements metadata routing for
SelfTrainingClassifier. Note the added code diff simply comes from a replacement ofbase_estimatorforestimator.NonConsumingClassifierfixes some minor design choices in the unit-testing framework oftest_metaestimators_metadata_routing.py(e.g.try/except->if/elseto be more transparent)Any other comments?
cc: @adrinjalali
Some open questions:
1. I presume, we want to forward metadata within all the functions possibly?2. As a result, I'm not sure if the unit-testing approach is the best, so I was wondering if you have any suggestions? Should I try potentially refactoring the existing unit-testing code to allow testing for more than justfit?