ARROW-11428: [Rust] Add power_scalar kernel #9361
Conversation
kou
changed the title
There was a problem hiding this comment.
This simplification makes this identical to crate::array::compute::kernels::arity::unary, which was recently added.
There was a problem hiding this comment.
Right.. Then there is some duplication. What do you propose? Use the crate::array::compute::kernels::arity::unary for the logic in this model as wel? To me that seems more DRY.
There was a problem hiding this comment.
@nevi-me I removed the helper function and used the arity::unary::kernel
rust/arrow/src/datatypes.rs
Outdated
There was a problem hiding this comment.
We could avoid creating this trait by using num::Float as a constraint for floaing types. See #9313, though I wasn't doing it for a SIMD implementation
There was a problem hiding this comment.
For non-SIMD this would work indeed. However I could not proof my SIMD type had the powf method without this trait/ implementation. Or am I missing something?
There was a problem hiding this comment.
You're right, if it's not possible to only add the trait for SIMD types, then it's a reasonable change to make.
The downside's that the trait's going to grow when we add more compute kernels (with SIMD support).
There was a problem hiding this comment.
Given that this function is powf, WDYT about renaming it as such? We're going to also have pow for the integer types, powi to raise floats to an integer power. So we could preempt the possible rename.
I'll remove the powf in #9313 because this one has a SIMD implementation.
An additional benefit of splitting powf and powi is that powi is faster.
There was a problem hiding this comment.
I renamed to powf_scalar. I still have a scalar suffix distinct with a (future) kernel that is a binary function (i.e. array to the power of array).
ritchie46
force-pushed
the
power_kernel
branch
from
27421f4 to
82c7535
Compare
nevi-me
left a comment
nevi-me
left a comment
There was a problem hiding this comment.
LGTM, we have a persistent failure in the integration tests, which doesn't make sense.
Can you please rebase, in case there's something else that's causing the error.
In the absence of other reviewers, I'm happy that we can merge this.
ritchie46
force-pushed
the
power_kernel
branch
from
82c7535 to
511eb65
Compare
|
Rebased! 👍 |
2 participants
Adds a SISD and SIMD kernel to raise a
Float32/64array to a power of ascalarof the same type. We could also make a thinsqrtwrapper.I also added a
unary_opfn toArrowNumerictype as this seemed the most generic way to implement this. Next PR I could add support for a binary version of this (e.g. array to the power of array).edit:
The
ArrowFloatNumericTypetrait was added because the Simd::powf is only available for float arrays (e.g.[f32, N],[f64, N]). However, the packed_simd crate doesn't expose this functionality via a trait, but directly on the type, hence the extra trait.