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

Optimize Cauchy sampling #493

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

MaximoB
Copy link
Contributor

@MaximoB MaximoB commented Jun 4, 2018

Adding an implementation to a very fast algorithm for calculating Cauchy random numbers.
My basic testing of plotting some histograms seems to indicate it is about as numerically stable as the current method we are using.
Not sure about how to do paper credits so I am looking for feedback about that too.

@vks
Copy link
Collaborator

vks commented Jun 5, 2018

Could you please provide some before/after benchmarks?

Not sure about how to do paper credits so I am looking for feedback about that too.

For consistency, I would just go with how it was done for the normal distribution.

@dhardy
Copy link
Member

dhardy commented Jun 6, 2018

  1. How accurate is this?
  2. Yes, benchmarks please
  3. Also include the DOI please

@dhardy dhardy added the D-review Do: needs review label Jun 6, 2018
@MaximoB
Copy link
Contributor Author

MaximoB commented Jun 6, 2018

  1. The paper is not exactly clear on this, but from what I can tell the maximum error in their approximation of the quartile function looks like ±0.002
  2. Benchmarks against the current code (current code pulled this morning)
test distr_cauchy_current        ... bench:      28,211 ns/iter (+/- 15,429) = 283 MB/s
test distr_cauchy_new            ... bench:      11,631 ns/iter (+/- 8,514) = 687 MB/s
  1. DOI is: 10.1145/50087.50094. Are you saying you would like me to add it to the credit in the documentation?

@dhardy
Copy link
Member

dhardy commented Jun 7, 2018

Yes, I found the article (but didn't manage to download it yet). But I think it's worth adding to the credits.

Sounds IMO like we should treat this in line with #494; keep the current implementation for now but consider adding this as a fast approximation under distributions::fast or something (still under discussion).

@dhardy dhardy added T-distributions Topic: distributions C-optimisation P-postpone Waiting on something else and removed D-review Do: needs review labels Jun 7, 2018
@dhardy dhardy mentioned this pull request Sep 16, 2019
19 tasks
@dhardy
Copy link
Member

dhardy commented Jan 29, 2024

@vks any suggestion on what we do with this?

@vks
Copy link
Collaborator

vks commented Jan 30, 2024

I would prefer to only maintain one implementation. Can we quantify whether the accuracy of the new implementation is better or worse?

@dhardy
Copy link
Member

dhardy commented Jan 30, 2024

Agreed. Then we (or someone) needs to look into the accuracy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-optimisation P-postpone Waiting on something else T-distributions Topic: distributions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants