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

Improve calculation of the scale parameter for the uniform float distribution. #1301

Closed

Conversation

WarrenWeckesser
Copy link
Collaborator

The calculation of scale for the uniform float distribution is moved to its own function and updated to avoid the problem that was pointed out in gh-1299. As part of the new implementation, the new utility methods lt_mask(), increase_masked() and utils_next_down() are added to the FloatSIMDUtils traits.

The improved method for computing the scale is explained in the comments in the function compute_scale().

@dhardy
Copy link
Member

dhardy commented Mar 20, 2023

Quick question from a bird's eye perspective: decrease_masked is used in three locations, but this only replaces one. Is it relevant elsewhere? (Also, is it even needed in new_inclusive?)

@WarrenWeckesser
Copy link
Collaborator Author

In the few tests that I did, new_inclusive didn't show the behavior reported in gh-1299, so I focused on fixing the behavior in new, and didn't look much beyond that. I'll take another look, and also I'll get caught up on gh-1289 and the paper linked there.

In the meantime, I'm marking this PR as draft.

@WarrenWeckesser WarrenWeckesser marked this pull request as draft March 21, 2023 10:54
@GUIpsp
Copy link

GUIpsp commented Apr 10, 2023

Hello - sorry for the late comment.

I don't have an example off the top of my head, but I can say that I saw the same behaviour with new_inclusive (but with different constants).

@WarrenWeckesser
Copy link
Collaborator Author

Thanks, @GUIpsp. If you happen to find example of the bad behavior with new_inclusive, be sure to note it here or in #1299.

@dhardy dhardy mentioned this pull request Oct 31, 2023
23 tasks
@dhardy
Copy link
Member

dhardy commented Jan 29, 2024

@WarrenWeckesser is this still on your to-do list?

@dhardy dhardy added the D-work-in-progress Do: draft or unfinished PR label Jul 10, 2024
@WarrenWeckesser
Copy link
Collaborator Author

Looking at this again... I don't think attempting to hack the bounds like this is worth it. Take a look at the comments in the "Notes" section of https://en.cppreference.com/w/cpp/numeric/random/uniform_real_distribution. In particular, follow the links to the bug reports for the various C++ compilers.

I think it is useful to have a standard uniform distribution on the half-open interval [0, 1), and that can be done reliably, but handling all the edges cases of the general uniform distribution on [a, b) where it is guaranteed that b will not be generated while maintaining the uniform probabilities of the points near the boundary b and ideally ensuring that the algorithm consumes a fixed number of samples from the underlying PRNG (i.e. no looping for an indefinite number of iterations) is a more involved problem than I can work on now.

@dhardy
Copy link
Member

dhardy commented Jul 15, 2024

Thanks for taking another look at this @WarrenWeckesser. Then I think we go for #1462.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-work-in-progress Do: draft or unfinished PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants