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-12404: [C++] Implement "random" nullary function that generates uniform random between 0 and 1 #11864

Closed
wants to merge 15 commits into from
Closed

ARROW-12404: [C++] Implement "random" nullary function that generates uniform random between 0 and 1 #11864

wants to merge 15 commits into from

Conversation

asuhan
Copy link
Contributor

@asuhan asuhan commented Dec 6, 2021

No description provided.

@github-actions
Copy link

github-actions bot commented Dec 6, 2021

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

@github-actions
Copy link

github-actions bot commented Dec 6, 2021

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

@asuhan asuhan force-pushed the asuhan/random_nullary branch 3 times, most recently from 1334b15 to d6d83ba Compare December 6, 2021 09:00
@pitrou
Copy link
Member

pitrou commented Dec 6, 2021

By the way, it's not obvious the complicated generate_canonical implementation is useful. Here is what Numpy uses:
https://github.com/numpy/numpy/blob/fefe3309c8253726d37c184137a2e058a14b12cc/numpy/random/_common.pxd#L67-L68

@pitrou
Copy link
Member

pitrou commented Dec 6, 2021

That said, it may compile down to the same formula after optimizations. I can't tell for sure.

@cyb70289
Copy link
Contributor

cyb70289 commented Dec 7, 2021

That said, it may compile down to the same formula after optimizations. I can't tell for sure.

+1 for the simple implementation. I believe they are same, at least in theory. It's nothing more than RNG64()/(2^64) or RNG32()/(2^32).

@asuhan asuhan force-pushed the asuhan/random_nullary branch 3 times, most recently from f41d9a4 to 9d327a4 Compare December 9, 2021 08:33
@asuhan asuhan marked this pull request as ready for review December 9, 2021 08:35
@asuhan
Copy link
Contributor Author

asuhan commented Dec 9, 2021

Simplified the formula and made the seed initialization static, I think it's ready for review now.

@asuhan asuhan force-pushed the asuhan/random_nullary branch 2 times, most recently from b1b7089 to aa13afe Compare December 9, 2021 09:41
cyb70289
cyb70289 reviewed Dec 9, 2021
View reviewed changes
cpp/src/arrow/compute/kernels/random.cc Outdated Show resolved Hide resolved
cpp/src/arrow/compute/kernels/random.cc Outdated Show resolved Hide resolved
@asuhan asuhan force-pushed the asuhan/random_nullary branch 6 times, most recently from 9689824 to fa81409 Compare December 16, 2021 08:32
@asuhan
Copy link
Contributor Author

asuhan commented Dec 16, 2021

I've made it return an array, with the length in the options.

cyb70289
cyb70289 reviewed Dec 17, 2021
View reviewed changes
cpp/src/arrow/compute/kernels/random.cc Outdated Show resolved Hide resolved
cpp/src/arrow/compute/kernels/random.cc Outdated Show resolved Hide resolved
cpp/src/arrow/compute/kernels/random.cc Outdated Show resolved Hide resolved
cpp/src/arrow/compute/kernels/scalar_random_test.cc Show resolved Hide resolved
cpp/src/arrow/compute/registry.cc Outdated Show resolved Hide resolved
@cyb70289
Copy link
Contributor

Please rebase to latest master branch and fix merge conflicts.

@asuhan
Copy link
Contributor Author

asuhan commented Dec 22, 2021

@pitrou I was hoping that RandomOptions will perhaps be automatically exported in the Python bindings, sounds like that's not the case. I'd prefer to leave it to a follow-up task since I don't have a Python-enabled build ready.

@asuhan asuhan force-pushed the asuhan/random_nullary branch 2 times, most recently from e356e9f to 51d01e3 Compare December 27, 2021 04:41
asuhan and others added 13 commits December 29, 2021 14:31
@asuhan
ARROW-12404: [C++] Implement "random" nullary function that generates…
49fa7ba 
… uniform random between 0 and 1
@asuhan
ARROW-12404: Add option to set the seed
e58335c
@asuhan
ARROW-12404: Add NullaryOptions base class for RandomOptions
ef04430
@asuhan
ARROW-12404: Use pcg64_fast and improve documentation
ec7fbb0
@asuhan
ARROW-12404: Allocate random_device and pcg64_fast on stack
5dba8b6
@asuhan
ARROW-12404: Rename and move RegisterRandom
941e875
@asuhan
ARROW-12404: Use a static seed generator
8795055
@asuhan
ARROW-12404: Fill values directly in the underlying buffer
320874d 
Bypass array builder API.
@cyb70289 @asuhan
code refine, add bench
97387f2
@cyb70289 @asuhan
fix conflicted symbol names in unity build
49f3fed
@asuhan
ARROW-12404: Address minor comments
ff2929b
@asuhan
ARROW-12404: Add tests for system initialized seed
763aa3c
@asuhan
ARROW-12404: Use seed_seq_from to initialize seed generator
7cb7031
@asuhan
Copy link
Contributor Author

asuhan commented Dec 29, 2021

Any additional feedback on this? I think I've addressed all comments

cyb70289
cyb70289 approved these changes Dec 30, 2021
View reviewed changes
Copy link
Contributor

@cyb70289 cyb70289 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @asuhan !
I created jira issue to track python bind
https://issues.apache.org/jira/browse/ARROW-15219

@cyb70289
Copy link
Contributor

@pitrou, do you have other comments?

pitrou
pitrou approved these changes Dec 30, 2021
View reviewed changes
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.

+1 from me. I just pushed two minor changes.

@pitrou pitrou closed this in ceaed97 Dec 30, 2021
@pitrou
Copy link
Member

pitrou commented Dec 30, 2021

Thank you very much for this, @asuhan !

@ursabot
Copy link

ursabot commented Dec 30, 2021 •
edited
Loading

Benchmark runs are scheduled for baseline = ab9528e and contender = ceaed97. ceaed97 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
[Finished ⬇️0.45% ⬆️0.0%] ursa-i9-9960x
[Failed ⬇️0.31% ⬆️0.09%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@kou
Copy link
Member

kou commented Dec 31, 2021

@github-actions crossbow submit debian-bullseye-amd64

@github-actions
Copy link

Revision: 4af2978

Submitted crossbow builds: ursacomputing/crossbow @ actions-1357

Task Status
debian-bullseye-amd64 Github Actions

@asfimport asfimport mentioned this pull request Dec 31, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonpace westonpace westonpace left review comments

@pitrou pitrou pitrou approved these changes

@cyb70289 cyb70289 cyb70289 approved these changes

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

Successfully merging this pull request may close these issues.

None yet
6 participants
@asuhan @pitrou @cyb70289 @ursabot @kou @westonpace