Skip to content

ARROW-13280: [R] Bindings for log and trig functions #10689

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.

Sign up for GitHub

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

Jump to bottom
Closed
thisisnic wants to merge 10 commits into from
Closed

ARROW-13280: [R] Bindings for log and trig functions #10689

thisisnic wants to merge 10 commits into from

Conversation

@thisisnic
Copy link
Member

@thisisnic thisisnic commented Jul 9, 2021

No description provided.

@github-actions
Copy link

github-actions bot commented Jul 9, 2021

https://issues.apache.org/jira/browse/ARROW-13280

@thisisnic
Copy link
Member Author

thisisnic commented Jul 9, 2021

This PR will need rebasing off #10686 when it's merged

@lidavidm
Copy link
Member

lidavidm commented Jul 12, 2021

Sorry for the delay, I've just merged #10686

@thisisnic thisisnic changed the title ARROW-13280: [R] Bindings for log and trig functions [WIP] ARROW-13280: [R] Bindings for log and trig functions Jul 14, 2021
@thisisnic
Copy link
Member Author

thisisnic commented Jul 14, 2021

Sorry for the delay, I've just merged #10686

All good, wasn't urgent! :)

nealrichardson
nealrichardson requested changes Jul 14, 2021
View reviewed changes
Copy link
Member

@nealrichardson nealrichardson left a comment

Choose a reason for hiding this comment

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

Looks good, just one request.

r/R/expression.R Outdated
Copy link
Member

@nealrichardson nealrichardson Jul 14, 2021

Choose a reason for hiding this comment

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

Now that these exist, we should define the Math group generic for Arrow objects, like we have for Ops. I made https://issues.apache.org/jira/browse/ARROW-13337 for that.

Copy link
Member

@nealrichardson nealrichardson Jul 14, 2021

Choose a reason for hiding this comment

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

Hmm, in R, log is not unary:

log <- function (x, base = exp(1))

So we should handle that correctly. If arrow doesn't support general log_baseN, (1) make sure there's a JIRA for that, and (2) map the cases that do exist (e, 10, 2, etc.) and for now error "not supported" for any other value of base.

Copy link
Member Author

@thisisnic thisisnic Jul 15, 2021

Choose a reason for hiding this comment

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

Done and opened https://issues.apache.org/jira/browse/ARROW-13345

@thisisnic thisisnic requested a review from nealrichardson July 15, 2021 12:47
nealrichardson
nealrichardson reviewed Jul 15, 2021
View reviewed changes
@asfimport asfimport mentioned this pull request Aug 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nealrichardson nealrichardson nealrichardson approved these changes

Assignees

No one assigned

Labels

Component: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@thisisnic @lidavidm @nealrichardson