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.

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

ARROW-14779: [C++] Add other common round mode names to RoundMode docs #11838

Closed
wants to merge 3 commits into from
Closed

ARROW-14779: [C++] Add other common round mode names to RoundMode docs #11838

wants to merge 3 commits into from

Conversation

vibhatha
Copy link
Collaborator

@vibhatha vibhatha commented Dec 2, 2021

In this draft PR, the docstrings were updated and referred to the wikipedia definitions: https://en.wikipedia.org/wiki/Rounding#Types_of_rounding

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

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

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

lidavidm
lidavidm approved these changes Dec 2, 2021
View reviewed changes
Copy link
Member

@lidavidm lidavidm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks! Would you like to undraft this? (edit: and remove the [WIP])

@vibhatha vibhatha changed the title ARROW-14779: [C++] Add other common round mode names to RoundMode docs [WIP] ARROW-14779: [C++] Add other common round mode names to RoundMode docs Dec 2, 2021
@vibhatha vibhatha marked this pull request as ready for review December 2, 2021 14:00
@vibhatha
Copy link
Collaborator Author

vibhatha commented Dec 2, 2021

Thanks! Would you like to undraft this? (edit: and remove the [WIP])

Done. Thanks!

@lidavidm
Copy link
Member

lidavidm commented Dec 2, 2021

Sorry, do you want to rebase/force-push to get CI to trigger again? (It skips everything when you have "WIP" in the PR title)

pitrou
pitrou reviewed Dec 2, 2021
View reviewed changes
cpp/src/arrow/compute/api_scalar.h Outdated
@@ -57,19 +57,19 @@ class ARROW_EXPORT ElementWiseAggregateOptions : public FunctionOptions {
enum class RoundMode : int8_t {
/// Round to nearest integer less than or equal in magnitude (aka "floor")
DOWN,
/// Round to nearest integer greater than or equal in magnitude (aka "ceil")
/// Round to nearest integer greater than or equal in magnitude (aka "ceiling")
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, "trunc" and "ceil" are common API names for this, for example in C, Python or Rust, so perhaps keep those names here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I will make those changes.

@vibhatha
Copy link
Collaborator Author

vibhatha commented Dec 2, 2021

Sorry, do you want to rebase/force-push to get CI to trigger again? (It skips everything when you have "WIP" in the PR title)

Sure I will make one.

@pitrou
Copy link
Member

pitrou commented Dec 2, 2021

@vibhatha Can you fix the lint failures? This can normally be done using archery lint --cpplint --fix once you have installed Archery.

@vibhatha
Copy link
Collaborator Author

vibhatha commented Dec 2, 2021

@vibhatha Can you fix the lint failures? This can normally be done using archery lint --cpplint --fix once you have installed Archery.

I updated the PR. CI in progress.

@lidavidm
Copy link
Member

lidavidm commented Dec 2, 2021

Thanks. The RTools failure appears to be a timeout, the AppVeyor test appears to be S3 tests flaking (ARROW-13957)

@lidavidm lidavidm closed this in 28e9bac Dec 2, 2021
@ursabot
Copy link

ursabot commented Dec 2, 2021 •
edited
Loading

Benchmark runs are scheduled for baseline = a8ed77e and contender = 28e9bac. 28e9bac is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.4% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pitrou pitrou pitrou approved these changes

@lidavidm lidavidm lidavidm approved these changes

Assignees
No one assigned
Labels
Component: C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vibhatha @lidavidm @pitrou @ursabot