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

GH-44903: [C++] Add the Expm1(exponent) scalar arithmetic function #44904

Merged
merged 4 commits into from
Dec 2, 2024

Conversation

felipecrv
Copy link
Contributor

@felipecrv felipecrv commented Dec 2, 2024

Expm1(exponent) is a more accurate way of computing Exp(exponent) - 1.0 for small values of exponent.

Rationale for this change

expm1(x) is specifically designed to compute exp(x)−1 more accurately, particularly for small x. It uses numerical techniques and approximations that minimize the loss of precision.

When x is very small (close to 0), exp(x) is approximately 1+x. Subtracting 1 from exp(x) (i.e., (exp(x)−1) can result in significant cancellation of significant digits due to floating-point arithmetic, leading to a loss of precision).

For example:

When x = 10^−8, exp(x) is close to 1 + 10^−8, so subtracting 1 leaves only the small 10^−8, which may lose accuracy due to floating-point limitations.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes and documentation was updated to list the new function.

`Expm1(exponent)` is a more accurate way of computing `Exp(exponent) - 1.0`
for small values of exponent.
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

LGTM in general. Just a couple minor comments.

cpp/src/arrow/compute/api_scalar.h Outdated Show resolved Hide resolved
cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc Outdated Show resolved Hide resolved
"[-0.6321205588285577, 0.0, null, 22025.465794806718]");
// Ordinary arrays (positive, negative, fractional, and zero inputs)
this->AssertUnaryOp(
expm1, "[-10.0, 0.0, 0.5, 1.0]",
Copy link
Member

Choose a reason for hiding this comment

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

Nit: since the main point of expm1 is to be more precise when the input is close to zero, shouldn't we focus on this use case here? Though of course this is all dependent on the stdlib's implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added 0.0 but you're right about the need for other tiny numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Dec 2, 2024
@github-actions github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Dec 2, 2024
@felipecrv felipecrv requested a review from pitrou December 2, 2024 16:45
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Dec 2, 2024
Copy link
Contributor

@zanmato1984 zanmato1984 left a comment

Choose a reason for hiding this comment

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

+1. Only one nit.

Comment on lines +1090 to +1094
const FunctionDoc expm1_doc{
"Compute Euler's number raised to the power of specified exponent, "
"then decrement 1, element-wise",
("If exponent is null the result will be null."),
{"exponent"}};
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we mention that "this is more accurate than directly computing exp(value) - 1" in the function doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered it, but I think it's better to keep this succinct. expm1 is very googlable and explaining why it's more accurate in a short phrase isn't easy.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Dec 2, 2024
@felipecrv
Copy link
Contributor Author

felipecrv commented Dec 2, 2024

R test failures seem unrelated. I will merge soon if no one is against it.

@felipecrv felipecrv added this to the 19.0.0 milestone Dec 2, 2024
@felipecrv felipecrv merged commit 5e476b3 into apache:main Dec 2, 2024
43 of 44 checks passed
@felipecrv felipecrv removed the awaiting changes Awaiting changes label Dec 2, 2024
@felipecrv felipecrv deleted the expm1 branch December 2, 2024 21:45
Copy link

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 5e476b3.

There were 132 benchmark results with an error:

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 32 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants