- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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(rust, python): allow nonstrict cast of categorical/enum to enum #14910
Conversation
I'm not sure how strictness works. It seems like typically during a cast you just return None when it fails to cast for any particular value but then I'm guessing (b/c I don't see it here) that elsewhere it compares the input and output for nulls and if there are nulls in the output that weren't null in the input then that's when the strictness flag comes into play. In this case the error was baked into the casting so that's why it didn't respect strict? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14910 +/- ##
==========================================
- Coverage 81.01% 81.01% -0.01%
==========================================
Files 1332 1332
Lines 172870 172843 -27
Branches 2458 2458
==========================================
- Hits 140054 140031 -23
+ Misses 32348 32344 -4
Partials 468 468 ☔ View full report in Codecov by Sentry. |
Yes. >>> s = pl.Series(["a"])
>>> s.cast(pl.Date, strict=False)
shape: (1,)
Series: '' [date]
[
null
]
>>> s.cast(pl.Int32, strict=False)
shape: (1,)
Series: '' [i32]
[
null
] There are some situations where >>> pl.Series([[1]]).cast(pl.Utf8, strict=False)
polars.exceptions.ComputeError: cannot cast List type (inner: 'Int64', to: 'String') However, I feel it's reasonable to expecte that a non-strict cast from one enum to another should be treated the same way as a cast of string to enum--values outside of the categories are converted to null. |
Sorry I know what strict is supposed to do. I meant I don't know about the implementation of it in rust. |
Ahh, sorry. Yes, here is the |
Failures are related chrono update and should be fixed by #14928. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap, good one. Thank you @mcrumiller .
Resolves #14900.
@c-peters the downside of this PR is that it no longer prints your nice error messages when a value falls outside of the target Enum's categories, and instead assigns it a null value. Strict (normal) casts will still properly fail the casts because of the new null values introduced, but non-strict casts now convert values outside to null. On the plus side, this reduces the code a bit and removes two levels of option/result indirection. Here is the example from the issue after this PR:
strict (regular) casting still fails: