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-44953: [R] Add R bindings for new compute functions #44971

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

amoeba
Copy link
Member

@amoeba amoeba commented Dec 9, 2024

Rationale for this change

Adds R bindings to newly added hyperbolic trig and Expm1 compute functions. See #44953. Bindings prefer calling _checked variants of compute functions where applicable which follows how existing bindings have been implemented.

What changes are included in this PR?

  • R bindings for hyperbolic trig compute functions
  • R bindings for Expm1(Exponent) compute function
  • Adds missing dplyr binding for base::atan
  • Adds missing documentation for atan (not directly related to this PR)
  • Updates to NEWS.md

Are these changes tested?

Yes.

Are there any user-facing changes?

New compute bindings in R.

@github-actions github-actions bot added the awaiting review Awaiting review label Dec 9, 2024
@amoeba
Copy link
Member Author

amoeba commented Dec 9, 2024

This PR branch is currently based on #44630 and I'll rebase and force-push once that's merged. I still need to:

  • Add the Expm1(Exponent) binding and
  • Verify both sets of bindings are tested adequately

@amoeba amoeba force-pushed the feature/GH-44953--r-compute-bindings branch from de2ca98 to dd01aae Compare December 10, 2024 00:31
@amoeba amoeba marked this pull request as ready for review December 10, 2024 19:29
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.

Thanks for this! One minor comment and one question

r/NEWS.md Outdated
Comment on lines 22 to 24
- Added missing dplyr binding for atan (#44953)
- Added bindings for hyperbolig trig functions sinh, cosh, tanh, asinh, acosh, and tanh (#44953)
- Added bindings for expm1 (#44953)
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't really matter but I wonder if collapsing these into one bullet (since there's one issue) would be better?

Copy link
Member Author

Choose a reason for hiding this comment

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

One bullet is fine, I'll change it.

Comment on lines +143 to +148
cosh = eval_array_expression("cosh", x),
sinh = eval_array_expression("sinh", x),
tanh = eval_array_expression("tanh", x),
acosh = eval_array_expression("acosh_checked", x),
asinh = eval_array_expression("asinh", x),
atanh = eval_array_expression("atanh_checked", x),
Copy link
Member

Choose a reason for hiding this comment

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

Mostly out of curiosity: did we miss these earlier? Or are they also new?

Copy link
Member Author

Choose a reason for hiding this comment

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

They're new as of #44630 which hasn't been released.

@github-actions github-actions bot added awaiting merge Awaiting merge awaiting review Awaiting review awaiting committer review Awaiting committer review and removed awaiting review Awaiting review awaiting merge Awaiting merge labels Dec 10, 2024
@@ -15,6 +15,8 @@
# specific language governing permissions and limitations
# under the License.

source("../../extra-tests/helpers.R")
Copy link
Member Author

Choose a reason for hiding this comment

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

@jonkeane: I wanted to use skip_if_version_less_than so I did this but I think the tests can't rely on anything that's buildignored (like the extra-tests folder is). I think I'd prefer to move these helpers into ./tests and have the files in ./extra-tests pull that helper in. Any strong preference?

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.

2 participants