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.

Sign up for GitHub

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

Jump to bottom

Fix fetch_openml parser warning #518

Merged
merged 8 commits into from Mar 31, 2023
Merged

Fix fetch_openml parser warning #518

merged 8 commits into from Mar 31, 2023

Conversation

LilianBoulard
Copy link
Member

Fixes #517

@LilianBoulard LilianBoulard added the bug Something isn't working label Mar 2, 2023
@LilianBoulard LilianBoulard self-assigned this Mar 2, 2023
@GaelVaroquaux
Copy link
Member

The proposed fix cannot work on older versions of scikit-learn (and the test are failing).

The correct fix is to test for the version of scikit-learn and add the argument only if the version of scikit-learn is recent enough.

@LilianBoulard
Copy link
Member Author

I know but even with the most recent version, the tests fail, and I'm trying to understand why.
I agree the fix I submitted was lazy, I'll look into the error :)

@GaelVaroquaux
Copy link
Member

GaelVaroquaux commented Mar 3, 2023 via email

@LilianBoulard
Copy link
Member Author

So the issue we have is related to scikit-learn/scikit-learn#25478.
The PR scikit-learn/scikit-learn#25511 has been submitted and should be ready for sklearn 1.2.2. I can preemptively submit a fix, but we should leave this PR open until it's released.

@LilianBoulard LilianBoulard marked this pull request as draft March 3, 2023 14:46
@GaelVaroquaux
Copy link
Member

Tests are passing. Is this mergeable? I noted that the PR is still in draft mode.

GaelVaroquaux
GaelVaroquaux requested changes Mar 13, 2023
View reviewed changes
Copy link
Member

@GaelVaroquaux GaelVaroquaux left a comment

Choose a reason for hiding this comment

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

One small suggestion

dirty_cat/datasets/_fetching.py Show resolved Hide resolved
@LilianBoulard LilianBoulard marked this pull request as ready for review March 14, 2023 09:25
GaelVaroquaux
GaelVaroquaux approved these changes Mar 14, 2023
View reviewed changes
Copy link
Member

@GaelVaroquaux GaelVaroquaux left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot!

@GaelVaroquaux
Copy link
Member

The code looks just right!

We have test failing on the main branch (not sure how we got there, we screwed up at some point). Out of good practice, I'll wait for these to be fixed, and rebase, so as to merge with passing tests.

jovan-stojanovic
jovan-stojanovic approved these changes Mar 31, 2023
View reviewed changes
Copy link
Member

@jovan-stojanovic jovan-stojanovic left a comment

Choose a reason for hiding this comment

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

Merging now the tests have passed.

@jovan-stojanovic jovan-stojanovic merged commit 2df490a into skrub-data:main Mar 31, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GaelVaroquaux GaelVaroquaux GaelVaroquaux approved these changes

@jovan-stojanovic jovan-stojanovic jovan-stojanovic approved these changes

Assignees

@LilianBoulard LilianBoulard

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sklearn.datasets.fetch_openml parser warning
3 participants
@LilianBoulard @GaelVaroquaux @jovan-stojanovic