Make error message uniform when calling get_feature_names_out before fit
#24916
Comments
|
Thank you for reporting this. What you propose, that is:
seems like the best solution to me. |
|
yep, adding a |
|
I agree with raising a |
|
@glemaitre , I would like to help solve this issue. Any tips you would like to share before I start ? |
|
The best would be to create a common test running for all estimators having |
|
I am going to work on this. I will share updates if any. |
|
Hi @JohnPCode, I would like to work on this issue. Are you still working on it ? Can I start working on it ? Let me know. |
|
@glemaitre , I have tried to run tests on all estimators with |
|
I don't think so. Otherwise, a pull request would be associated with this issue. |
|
@glemaitre Do you actually expect some kind of pytest test to be implemented ? |
Yes I expect to have a common test where we make sure to raise a |
|
@AlexBuzenet, I created a common test and ran it on all estimators with (sklearn-dev) john@SwiftyXSwaggy:~/Documents/Local_Code/scikit-learn$ pytest -vsl sklearn/tests/test_common.py -k error_check_get_feature_names_out ============================================================================ test session starts ============================================================================ platform linux -- Python 3.9.15, pytest-7.2.0, pluggy-1.0.0 -- /home/john/miniconda3/envs/sklearn-dev/bin/python3.9 cachedir: .pytest_cache rootdir: /home/john/Documents/Local_Code/scikit-learn, configfile: setup.cfg plugins: cov-4.0.0, xdist-3.1.0 collected 9597 items / 9400 deselected / 197 selected |
|
@jpangas Maybe you can start with one PR with the new test in test_common.py ? We will need it to verify that the fixes are correctly implemented. |
|
I agree with @albuzenet . You can start with one PR that contains the test which runs for all estimators (in a parameterized way). scikit-learn/sklearn/tests/test_docstrings.py Line 379 in 670133d If you wish to collaborate on this, hit me up on scikit-learn discord channel :) |
|
Update: The test that checks and ensures a |
|
(The content of this message had been moved to the description of this PR.) |
|
Working on AdditiveChi2Sampler, StackingClassifier and StackingRegressor, IterativeImputer, KNNImputer, SimpleImputer |
|
Working on estimators that inherit from |
|
@adrinjalali and @glemaitre, please help review the changes in PR #25308 and #25324 |
|
Thanks @rprkh ,I will now work on the final estimators so we can finally close this issue. |
|
@jpangas May I work on KBinsDiscretizer, MissingIndicator ? I'd like to contribute before the issue is closed ? |
|
@albuzenet , Sure go ahead and work on those estimators. It would be better if you could work on SplineTransformer and Dict Vectorizer as well. With one PR, we can finally close the issue without need for multiple reviews. |
|
@jpangas I was going through the list of estimators in #24916 (comment) and I think need to include these ones as well: scikit-learn/sklearn/tests/test_common.py Lines 489 to 490 in bf03a63 I'll make a PR for this. |
|
@rprkh, I see you have included the changes for |
|
Sure @jpangas, I'll update the PR by EOD. |
|
@jpangas: is it OK for you if I move the list you wrote in #24916 (comment) in the description of this issue so that we have a better overview and a list of subtasks? |
|
@jjerphan, yes. It's absolutely okay. Thank you. It will help track easily. |
|
Thanks @albuzenet and @rprkh for working alongside this issue with me. Thanks @glemaitre , @adrinjalali, @thomasjpfan , @jjerphan for the guidance and quick reviews. Onto the next. 💪 |
|
It was nice collaborating with everyone. Thank you for the help and quick reviews🙂 |
While working #24838, we found out that we are not consistent with the error type and message when calling
get_feature_names_outbeforefit.From @jpangas:
I assume that the most adequate error should be a
NotFittedErrorasking to fit the estimator.@scikit-learn/core-devs WDYT?
The text was updated successfully, but these errors were encountered: