Skip to content

API move BaseEstimator._validate_data to utils.validation.validate_data#29696

Merged
adrinjalali merged 17 commits into
scikit-learn:mainfrom
adrinjalali:validate_data2
Sep 5, 2024
Merged

API move BaseEstimator._validate_data to utils.validation.validate_data#29696
adrinjalali merged 17 commits into
scikit-learn:mainfrom
adrinjalali:validate_data2

Conversation

@adrinjalali

@adrinjalali adrinjalali commented Aug 21, 2024

Copy link
Copy Markdown
Member

Alternative to and closes #29672

cc @thomasjpfan @glemaitre

@adrinjalali adrinjalali added this to the 1.6 milestone Aug 21, 2024
@adrinjalali adrinjalali added the Developer API Third party developer API related label Aug 21, 2024
@github-actions

github-actions Bot commented Aug 21, 2024

Copy link
Copy Markdown

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 5d7f6e7. Link to the linter CI: here

@glemaitre glemaitre left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still uncertain about the naming of the function that does not reflect the mutation of estimator. validate_data_and_set_estimator will be an annoying name :)

I assume that we don't have an easy way to decoupled the data validation from the estimator mutation?

Comment thread sklearn/utils/validation.py Outdated
Comment thread sklearn/utils/validation.py Outdated
Comment thread sklearn/utils/validation.py
Comment thread sklearn/utils/validation.py

@glemaitre glemaitre left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM then.

@adrinjalali

Copy link
Copy Markdown
Member Author

@adam2392 @Charlie-XIAO @OmarManzoor this should be a relatively easy one to review.

@adam2392 adam2392 self-requested a review September 3, 2024 15:03

@Charlie-XIAO Charlie-XIAO left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, thanks @adrinjalali.

We may want to add an entry in doc/api_reference.py to reveal this public API in the docs (then I may review the rendering of the docstring).

@adrinjalali

Copy link
Copy Markdown
Member Author

@Charlie-XIAO Charlie-XIAO left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @adrinjalali, here are some minor fixes to the docstring.

Comment thread sklearn/utils/validation.py Outdated
Comment thread sklearn/utils/validation.py Outdated
Comment thread sklearn/utils/validation.py Outdated
Comment thread sklearn/utils/validation.py
Comment thread sklearn/utils/validation.py Outdated
Comment thread sklearn/utils/validation.py Outdated
Comment on lines +2819 to +2821
Only used if y is not None.
If False, call validate_X_y(). Else, it must be a tuple of kwargs
to be used for calling check_array() on X and y respectively.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Only used if y is not None.
If False, call validate_X_y(). Else, it must be a tuple of kwargs
to be used for calling check_array() on X and y respectively.
Only used if `y` is not `None`.
If `False`, call `validate_X_y()`. Else, it must be a tuple of kwargs
to be used for calling `check_array` on `X` and `y` respectively.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is validate_X_y even a function within sklearn anymore? I can't find it anywhere.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it's check_X_y

@adam2392 adam2392 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps these will help link the different functions together?

Comment thread sklearn/utils/validation.py Outdated
Comment thread sklearn/utils/validation.py Outdated
@adrinjalali

Copy link
Copy Markdown
Member Author

@adam2392 @Charlie-XIAO this should be good now

@adam2392 adam2392 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment thread sklearn/utils/validation.py Outdated
`estimator=self` is automatically added to these dicts to generate
more informative error message in case of invalid input data.

cast_to_ndarray : bool, default=True

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adrinjalali I did not think too hard about the cast_to_ndarray parameter name when _valdiate_data was private. Do you see a better name for cast_to_ndarray?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what this array in practice is doing is skip_check_array really. So we could call it that? With Array API and sparse arrays and all that, I don't think "casting to ndarray" is quite accurate.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like skip_check_array a lot more than cast_to_ndarray.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@Charlie-XIAO Charlie-XIAO left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, a few more suggestions below:

Comment thread sklearn/utils/validation.py Outdated
Comment thread sklearn/utils/validation.py Outdated
Comment thread sklearn/utils/validation.py Outdated
Co-authored-by: Yao Xiao <108576690+Charlie-XIAO@users.noreply.github.com>
adrinjalali and others added 4 commits September 5, 2024 16:15
Co-authored-by: Yao Xiao <108576690+Charlie-XIAO@users.noreply.github.com>
Co-authored-by: Yao Xiao <108576690+Charlie-XIAO@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Developer API Third party developer API related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants