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

Add Gen::gen_uniform #283

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

audunska
Copy link

@audunska audunska commented Mar 26, 2021

Arbitrary impls for nested data often needs access to some kind of random numbers. To avoid exposing rand types, a simple uniformly distributed float on the unit interval is often sufficient, as many distributions over floats and integers can be generated from this.

See #279.

@@ -76,6 +76,11 @@ impl Gen {
{
self.rng.gen_range(range)
}

/// Create a random number uniformly distributed on the unit interval [0, 1)
pub fn gen_uniform(&mut self) -> f32 {
Copy link

Choose a reason for hiding this comment

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

That should probably be gen_uniform_f32 or somesuch, since there could as well be versions for f64, and all the integer types.

Copy link
Author

Choose a reason for hiding this comment

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

I guess one could make it polymorphic using num-traits..

@kstrafe
Copy link

kstrafe commented Jul 11, 2021

Why can't we have gen_range instead to generate a uniform range? 0f32..=3f32 does not expose a rand type for instance.

@kstrafe
Copy link

kstrafe commented Jul 11, 2021

Another note: Scaling up a uniform f32 [0, 1) range to [1, 10) via x*9 + 1 leaves holes. There exists no float in [0, 1) that maps to 7.256350994110107421875

@akesling
Copy link

akesling commented Aug 23, 2021

Why can't we have gen_range instead to generate a uniform range? 0f32..=3f32 does not expose a rand type for instance.

Why not just make the existing Gen::gen_range public? It's already used extensively in the provided Arbitrary implementations to good effect.

EDIT: I should have read up the comment chain first. #267 (comment) already discusses gen_range being made public. The conclusion (as implied by the OP) is that it is apparently undesirable to make rand a public dependency.

@BurntSushi
Copy link
Owner

Because it introduces a public dependency on rand.

@akesling
Copy link

akesling commented Aug 23, 2021

Because it introduces a public dependency on rand.

@BurntSushi Have you considered updating included implementations of Arbitrary to only use the public interface?

Doing so should help clarify by example how people should go about creating their own impls. Having "internal" implementations use gen_range and then not providing it publicly was... disappointing :(.

@BurntSushi
Copy link
Owner

I've already acknowledged the deficiency elsewhere. I'm not ignorant of the problem. Ignorance isn't what is holding back improvement here. Time to dedicate toward maintenance is.

@akesling
Copy link

I've already acknowledged the deficiency elsewhere. I'm not ignorant of the problem. Ignorance isn't what is holding back improvement here. Time to dedicate toward maintenance is.

@BurntSushi apologies if I've offended. I'm the ignorant participant in this thread just trying to understand the state of the world to use the library (and maybe help others do the same). Do you have a link to discussion of changing the included implementations of Arbitrary? The more cross-linked these discussions become, the harder it is for people like me to ask stupid questions.

Also, would you be open to a PR that migrates some Arbitrary impls to only the public interface?

@BurntSushi
Copy link
Owner

BurntSushi commented Aug 23, 2021

Again, the problem here is my available time to do maintenance. Organizing discussions is maintenance. And changing the Arbitrary impls to use only public APIs is not only impossible but a waste of my time (and also requires the time to do maintenance). That is, I don't know what benefit it would bring. Unless you're proposing adding new APIs to Gen, in which case, that could be worthwhile, yes. But you'll want to see my comments on this topic in recent PRs and issues first.

I'm sorry but you'll have to look at recently opened issues and PRs to understand the state of the world here. There aren't that many.

@BurntSushi
Copy link
Owner

I made a couple of edits to my last comment in case you're reading via email, FYI.

@akesling
Copy link

For anyone who intersects with this discussion and needs the context I was lacking, start by reading #241 and #265 .

As it seems I've overstayed my welcome in this thread, I'll do my best to read a proper history of all of this (which I tried but apparently failed to do before posting initially). Once there, I'll comment on whatever open PR/Issue is more pertinent, or, as per the request in #265, open a more relevant issue.

@BurntSushi thank you for taking the time to help contextualize all of this. I'm a fan of your work and can appreciate the burden of so much public code to manage.

@BurntSushi
Copy link
Owner

OK, I'm finally at a workstation.

I personally think this is the most promising direction: #277

There is even a PR for it: #278

I just haven't had time to sit down, digest and commit to it. That's what I meant earlier by the problem being bottlenecked on me and my time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants