Skip to content
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

FIX make dataset fetchers accept os.Pathlike for data_home #27468

Merged
merged 9 commits into from Sep 28, 2023

Conversation

Charlie-XIAO
Copy link
Contributor

Reference Issues/PRs

Fixes #27447.

What does this implement/fix? Explain your changes.

This PR changes the parameter constraints of the dataset fetchers from [str, None] to [str, os.PathLike, None].

Any other comments?

The functions do have the ability to handle os.PathLike, but previously validate_params is disabling it. The unit test also only makes sure that it is able to handle these data_home, but does check explicitly for each data fetcher. Not sure if this is sufficient.

@github-actions
Copy link

github-actions bot commented Sep 26, 2023

✔️ Linting Passed

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

Generated for commit: 79e99eb. Link to the linter CI: here

@glemaitre glemaitre self-requested a review September 28, 2023 08:05
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

LGTM on my side. Thanks @Charlie-XIAO

@adrinjalali do you want to have a look.

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Happy to merge, since this was supported already anyway. But I'm not sure if we really support ANY pathlike object though.

@adrinjalali adrinjalali merged commit 2a548da into scikit-learn:main Sep 28, 2023
27 checks passed
lesteve pushed a commit to lesteve/scikit-learn that referenced this pull request Sep 28, 2023
…t-learn#27468)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@Charlie-XIAO Charlie-XIAO deleted the fetch-accept-pathlike branch September 30, 2023 08:50
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Oct 17, 2023
…t-learn#27468)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
glemaitre added a commit that referenced this pull request Oct 23, 2023
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
…t-learn#27468)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accept pathlib.Path for data_home in fetch_openml
3 participants