8000 TST Fix openml parser implementation for pandas-dev by thomasjpfan · Pull Request #26386 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

TST Fix openml parser implementation for pandas-dev #26386

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

Merged
merged 2 commits into from
May 23, 2023

Conversation

thomasjpfan
Copy link
Member
@thomasjpfan thomasjpfan commented May 16, 2023

Reference Issues/PRs

Partial Addresses #26154
Alternative to #26344

What does this implement/fix? Explain your changes.

This PR uses the second chunk to infer the dtypes and uses those types for the first chunk. For the titanic dataset, this means that body feature gets correctly inferred as a numeric dtype. OpenML denotes body as a numeric feature in it's metadata.

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.

This looks better to me. Does it need a changelog? It might be changing dtypes in some cases probably?

@glemaitre
Copy link
Member

I would consider this a fix because we have better dtypes. We should have an entry in what's new to acknowledge it.

< 8000 details open="open" data-resolved="false" data-target="details-collapsible.detailsElement details-toggle.detailsTarget" data-view-component="true" class="review-thread-component js-comment-container js-resolvable-timeline-thread-container Details-element details-reset mb-3 border rounded-2">
@@ -925,7 +925,7 @@ def datasets_missing_values():
# with casting it will be transformed to either float or Int64
(40966, "pandas", 1, 77, 0),
# titanic
(40945, "liac-arff", 3, 5, 0),
Copy link
Member

Choose a reason for hiding this comment

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

I am a bit confused by this PR: why would fixing the pandas parser impact a test for the liac-arff parser?

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR is fixing the liac-arff parser which uses pandas when as_frame=True.

For this specific tests, the "body" feature is interpreted as a object dtype on main. With this PR, the "body" feature is a float dtype, which increases the expected_n_floats by one.

@lesteve
Copy link
Member
lesteve commented May 23, 2023

Merging, thanks a lot!

@lesteve lesteve merged commit edfdbd0 into scikit-learn:main May 23, 2023
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
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.

5 participants
2A28
0