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: Return correct dtype for s.clear() when dtype is Object #15315

Merged
merged 10 commits into from
Mar 28, 2024

Conversation

mcrumiller
Copy link
Contributor

Resolves #15303.

I think this is a good way to do it? @stinodego will need your input here.

@github-actions github-actions bot added fix Bug fix python Related to Python Polars rust Related to Rust Polars labels Mar 26, 2024
@mcrumiller
Copy link
Contributor Author

mcrumiller commented Mar 26, 2024

Also, any reason we cannot simply do a length-0 slice and clone? That works too. Update: using slice method instead to prevent other errors.

Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 81.36%. Comparing base (3785fad) to head (c860e97).
Report is 11 commits behind head on main.

❗ Current head c860e97 differs from pull request most recent head 51345cb. Consider uploading reports for the commit 51345cb to get more accurate results

Files Patch % Lines
crates/polars-core/src/series/mod.rs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #15315      +/-   ##
==========================================
- Coverage   81.41%   81.36%   -0.06%     
==========================================
  Files        1362     1364       +2     
  Lines      176734   176613     -121     
  Branches     2531     2526       -5     
==========================================
- Hits       143896   143696     -200     
- Misses      32354    32434      +80     
+ Partials      484      483       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@stinodego stinodego left a comment

Choose a reason for hiding this comment

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

Thanks, I think this is a good fix. But I don't know who wrote the original 'hack' - so I'm not sure their use case is now covered. @ritchie46 could you take a quick look?

self.clone()
} else {
// SAFETY: we can always take zero elements
unsafe { self.take_slice_unchecked(&[]).clone() }
Copy link
Member

Choose a reason for hiding this comment

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

This works. Though this is not worth doing unsafe. Can we use self.take.

I also wonder why we need to clone afterwards? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do. Out of curiosity, is this path a bit simpler/faster than the standard dtype path of going through full_null with zero elements? We could just remove all the if logic and just return an empty slice for all dtypes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh unfortunately that breaks pl.Array. I'll keep the special object logic in there.

@mcrumiller
Copy link
Contributor Author

mcrumiller commented Mar 27, 2024

@ritchie46 used take_slice instead of unchecked now. Moved the empty clone check to all dtypes since it'll be a little faster and simplifies a bit.

Edit: this broke some memory alignment on Windows build for some reason.

@mcrumiller
Copy link
Contributor Author

Ok, no more errors.

@ritchie46
Copy link
Member

slice doesn't cut it. A slice doesn't clear memory.

@mcrumiller mcrumiller marked this pull request as draft March 28, 2024 13:50
@mcrumiller mcrumiller marked this pull request as ready for review March 28, 2024 14:56
@mcrumiller mcrumiller requested a review from reswqa as a code owner March 28, 2024 14:56
@mcrumiller
Copy link
Contributor Author

@ritchie46 this one passes now and uses take. I moved the clone up front for all dtypes since that's probably a bit faster than going through full_null when the series is already empty.

@ritchie46 ritchie46 merged commit 3888b86 into pola-rs:main Mar 28, 2024
23 checks passed
@mcrumiller mcrumiller deleted the object-clear branch March 28, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using clear on Series of type Object results in dtype Null when first value is null
3 participants