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

UnitCircle including the inside #797

Closed
kuviman opened this issue May 15, 2019 · 4 comments · Fixed by #798
Closed

UnitCircle including the inside #797

kuviman opened this issue May 15, 2019 · 4 comments · Fixed by #798

Comments

@kuviman
Copy link

kuviman commented May 15, 2019

I would like to produce random points inside a circle.

A UnitCircleInside and UnitSphereInside distributions would be great to have.

Also, there is currently UnitCircle distribution, but it only produces points on edge. And this is not obvious since there is also UnitSphereSurface which states such fact in the name. So, ideally UnitCircle could be renamed to UnitCircleEdge.

@dhardy
Copy link
Member

dhardy commented May 15, 2019

Actually, we already discussed the names: 'circle' typically refers to just the edge, while 'disc' refers to the internal area. For a sphere, the edge might be called a 'shell', but that name appeared more confusing, hence the desicision to use UnitSphereSurface. So we can use:

  • UnitDisc
  • UnitSphereInterior

See the discussion in #567. Implementing these should be straightforward, and I don't see why they shouldn't be added (to rand_distr), but @vks may also wish to comment.

@kuviman
Copy link
Author

kuviman commented May 15, 2019

Hm, 'sphere' also usually refers to the shell, and the interior is called 'ball', I think?

@vks
Copy link
Collaborator

vks commented May 16, 2019

Sampling from inside a circle or sphere is most efficiently done using rejection sampling, so it is very easy to implement (see the corresponding benchmarks). I'm fine with adding them to rand_distr. They were not yet added, because I was not sure how useful it would be, and because their implementations are much more straightforward than for sampling from the surface.

@dhardy
Copy link
Member

dhardy commented May 16, 2019

Those appear to be the terms used by Wikipedia and Wolfram at least; I'm happy to switch to this terminology. @vks? This is actually a good time to rename UnitSphere since rand_distr already has breaking changes.

Another thing occuring to me is that we could abstract over dimensions as follows, though probably this is more confusing than useful:

impl Distribution<[f64; 2]> for UnitSphere { .. }
impl Distribution<[f64; 3]> for UnitSphere { .. }

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 a pull request may close this issue.

3 participants