Uh oh!
There was an error while loading. Please reload this page.
ARROW-13345: [C++] Add basic implementation for log to base b - #10898
ARROW-13345: [C++] Add basic implementation for log to base b#10898rommelDB wants to merge 7 commits into
Conversation
Is it useful for the log base to be given as a |
rommelDB
commented
Aug 9, 2021
@pitrou To be honest, I'm not sure if that such a scenario exists or whether that scenario is frequent. In any case, this draft was my first approach, and I can certainly change it to consider the base as a single value as a function option. Accordingly, I think that the base as a value makes more sense. |
pitrou
commented
Aug 9, 2021
jonkeane
commented
Aug 9, 2021
That seems pretty uncommon to me. I could contrive a few examples that aren't totally outlandish (say you're scaling/normalizing a value by groups, you might save the log base that you did the normalization with to be able to transform something back into the original values. But that's pretty contrived, and I bet most people would approach that with a lookup table and a for loop, basically. |
ianmcook
commented
Aug 10, 2021
I like having the option for the base to be an array. I suspect that in the vast majority of cases users will pass a scalar base, but this implementation is the most flexible way to do it. |
pitrou
commented
Aug 10, 2021
That sounds fine to me. |
lidavidm
left a comment
There was a problem hiding this comment.
Thanks, looks good to me overall! I left some comments on the test cases.
| auto func = | ||
| std::make_shared<ArithmeticFloatingPointFunction>(name, Arity::Binary(), doc); | ||
| for (const auto& ty : FloatingPointTypes()) { | ||
| auto output = is_integer(ty->id()) ? float64() : ty; |
There was a problem hiding this comment.
nit: this line is redundant now (ty is never integer). I had filed ARROW-13361 to clean this up.
| const FunctionDoc logb_doc{ | ||
| "Compute log of x to base b of arguments element-wise", | ||
| ("Values <= -1 return -inf or NaN. Null values return null.\n" |
| } | ||
| TYPED_TEST(TestBinaryArithmeticFloating, Log) { | ||
| // Integer arguments promoted to double, sanity check here |
There was a problem hiding this comment.
nit: I don't think this comment applies here
| this->AssertBinop(Logb, "[1.0, 10.0, null]", "[10.0, 10.0, null]", "[0.0, 1.0, null]"); | ||
| this->AssertBinop(Logb, "[1.0, 2.0, null]", "[2.0, 2.0, null]", "[0.0, 1.0, null]"); | ||
| this->AssertBinop(Logb, "[10.0, 100.0, 1000.0, null]", this->MakeScalar(10), | ||
| "[1.0, 2.0, 3.0, null]"); |
There was a problem hiding this comment.
We should also test what happens on Log of NaN, Inf, and -Inf. We should also check the error cases below here, with both check_overflow = true and check_overflow = false. Or really, it should look similar to the tests for the unary log functions here:
There was a problem hiding this comment.
More unit tests were added.
pitrou
commented
Aug 12, 2021
@rommelDB Can you try to fix the build errors on Windows (see CI)? |
lidavidm
left a comment
There was a problem hiding this comment.
LGTM. As Antoine notes, there's a build failure on Windows, you'll likely need a cast somewhere: https://github.com/apache/arrow/pull/10898/checks?check_run_id=3306264947#step:7:901
lidavidm
commented
Aug 13, 2021
Thanks for implementing this! |
Changes: