-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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(python): Fix cast Float to String where Float is not turn to Integer before turning to String #18123
fix(python): Fix cast Float to String where Float is not turn to Integer before turning to String #18123
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18123 +/- ##
=======================================
Coverage 80.33% 80.33%
=======================================
Files 1496 1496
Lines 197685 197693 +8
Branches 2821 2821
=======================================
+ Hits 158806 158820 +14
+ Misses 38358 38352 -6
Partials 521 521 ☔ View full report in Codecov by Sentry. |
pl.select(pl.lit(float, dtype=pl.Utf8))
to not round down @@ -536,8 +536,40 @@ impl<'a> AnyValue<'a> { | |||
(AnyValue::Float64(v), DataType::Boolean) => AnyValue::Boolean(*v != f64::default()), | |||
|
|||
// to string | |||
(av, DataType::String) => { | |||
AnyValue::StringOwned(format_smartstring!("{}", av.extract::<i64>()?)) | |||
(AnyValue::UInt8(_v), DataType::String) => { |
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.
I don't think we need full code branches for every type.
We can check if the av
is float and in that case extract f64
. We should add a case for u64
. For the other cases we can keep i64
extraction.
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.
Ok I can do
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.
Much cleaner! Thanks @EricTulowetzke
closes #17773
Fixed:
This is my first time doing a PR for this project so set it to draft to hear feedback. If I keep the below line in test where we use agg, max, min break for Categorical data.
(_, DataType::String) => { AnyValue::StringOwned(format_smartstring!("{}", self.extract::<i64>()?)) },
I can add more cases for string casting if that is wanted so we define all types.