Skip to content

Conversation

@aboyoun
Copy link
Contributor

@aboyoun aboyoun commented Jul 6, 2024

Rationale for this change

When support was added for cumsum in the Math group generics it mistakenly mapped signif, expm1, log1p, cospi, sinpi, tanpi, cosh, sinh, tanh, acosh, asinh, atanh, lgamma, gamma, digamma, and trigamma to the cumulative_sum_checked arrow function. This PR corrects that mistake and well as adds support for log2, log1p, cumprod, cummax, and cummin.

What changes are included in this PR?

It contains the following changes:

  1. acos, asin, cos, sin, tan now map to the *_checked arrow function variants
  2. log2 maps to the log2_checked arrow function
  3. log1p maps to the log1p_checked arrow function
  4. cumprod maps to the cumulative_prod_checked arrow function
  5. cummax maps to the cumulative_max arrow function
  6. cummin maps to the cumulative_min arrow function
  7. signif, expm1, cospi, sinpi, tanpi, cosh, sinh, tanh, acosh, asinh, atanh, lgamma, gamma, digamma, and trigamma properly throw an unsupported operation error

Are these changes tested?

Yes, tests were added to "Math group generics work on Array objects" in arrow/r/tests/testthat/test-compute-arith.R

Are there any user-facing changes?

No

…max and fix bindings after support was added for cumsum
@github-actions
Copy link

github-actions bot commented Jul 6, 2024

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@aboyoun aboyoun changed the title [R] Fix bindings in Math group generics GH-43163 [R] Fix bindings in Math group generics Jul 6, 2024
@aboyoun aboyoun changed the title GH-43163 [R] Fix bindings in Math group generics GH-43163: [R] Fix bindings in Math group generics Jul 6, 2024
@github-actions
Copy link

github-actions bot commented Jul 6, 2024

⚠️ GitHub issue #43163 has been automatically assigned in GitHub to PR creator.

Copy link
Member

@jonkeane jonkeane left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! Both the issue and the PR are great, detailed, to the point, and with additional tests.

I'm going to (tentatively) approve this, though will wait for CI to pass before merging — if there's any issue there I might ask for more changes

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Jul 7, 2024
@jonkeane jonkeane merged commit 8e5c4e6 into apache:main Jul 7, 2024
@jonkeane jonkeane removed the awaiting merge Awaiting merge label Jul 7, 2024
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 8e5c4e6.

There were 5 benchmark results indicating a performance regression:

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

jonkeane pushed a commit that referenced this pull request Jul 18, 2024
### Rationale for this change

When support was added for `cumsum` in the `Math` group generics it mistakenly mapped `signif`, `expm1`, `log1p`, `cospi`, `sinpi`, `tanpi`, `cosh`, `sinh`, `tanh`, `acosh`, `asinh`, `atanh`, `lgamma`, `gamma`, `digamma`, and `trigamma` to the `cumulative_sum_checked` arrow function. This PR corrects that mistake and well as adds support for `log2`, `log1p`, `cumprod`, `cummax`, and `cummin`.

### What changes are included in this PR?

It contains the following changes:
1. `acos`, `asin`, `cos`, `sin`, `tan` now map to the `*_checked` arrow function variants
2. `log2` maps to the `log2_checked` arrow function
3. `log1p` maps to the `log1p_checked` arrow function
4. `cumprod` maps to the `cumulative_prod_checked` arrow function
5. `cummax` maps to the `cumulative_max` arrow function
6. `cummin` maps to the `cumulative_min` arrow function
7. `signif`, `expm1`, `cospi`, `sinpi`, `tanpi`, `cosh`, `sinh`, `tanh`, `acosh`, `asinh`, `atanh`, `lgamma`, `gamma`, `digamma`, and `trigamma` properly throw an unsupported operation error

### Are these changes tested?

Yes, tests were added to "Math group generics work on Array objects" in `arrow/r/tests/testthat/test-compute-arith.R`

### Are there any user-facing changes?

No

* GitHub Issue: #43163

Authored-by: Patrick Aboyoun <aboyoun.patrick@gene.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
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.

3 participants