DOC improve doc for _check_n_features and _check_feature_names and fix their usages
#31585
Conversation
…mes usage Add docstrings suggesting validate_data(skip_check_array=True) as the preferred option. Add inline comment to explain usage in FunctionTransformer.
| # Usage of _check_n_features and _check_feature_names here aligns with | ||
| # validate_data(..., skip_check_array=True), which avoids full validation. |
There was a problem hiding this comment.
can't we do validate_data(..., skip_check_array=True) instead?
There was a problem hiding this comment.
Thanks! You're absolutely right — in principle, validate_data(..., skip_check_array=True) could replace these internal calls.
In this PR, I kept the original usage to avoid changing existing logic, since the primary goal was documentation improvement and clarification. If preferred, I'm happy to open a follow-up PR that refactors the usage to use validate_data() directly and confirms everything still behaves correctly.
Let me know how you'd like to proceed!
There was a problem hiding this comment.
I think it'd be nice to change the logic here in this PR.
There was a problem hiding this comment.
Thanks for the suggestion @adrinjalali! I’ve updated _check_input to use validate_data(..., skip_check_array=True) directly as you recommended, replacing the internal checks.
I also tested it locally using Python 3.10.11 — the file test_function_transformer.py passed without any issues. Let me know if anything else looks off!
_check_n_features and _check_feature_names and fix their usages
| _check_n_features(self, X, reset=reset) | ||
| _check_feature_names(self, X, reset=reset) | ||
| # Use validate_data to perform shape and feature name checks | ||
| return validate_data(self, X, reset=reset, skip_check_array=True) |
There was a problem hiding this comment.
I think this whole method (_check_input) can be removed and replaced with a validate_data(..., skip_check_array=self.validate) kinda call.
There was a problem hiding this comment.
Hi! Just wanted to note that a few CI jobs are failing due to the stricter validation added by replacing _check_input() with validate_data(...).
Specifically, test_transform_target_regressor_1d_transformer now raises a ValueError because it passes an empty input X, and validate_data correctly catches that.
Let me know if you'd like me to update the test or handle it a different way — happy to make the changes!
Thanks for helping finish this up @jeremiedbb — sorry I missed your last round of feedback, but I appreciate the merge and the improvements! 😊 |
…cikit-learn#31585) Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
Towards #30389
This PR improves the documentation for _check_n_features and _check_feature_names by:
Adding clear docstrings to guide users toward using validate_data(skip_check_array=True) as the preferred public interface.
Including a clarifying comment in FunctionTransformer._check_input explaining that the use of these internal functions aligns with validate_data(..., skip_check_array=True) and is appropriate in this context.