Fibonacci Sampling Method #1092
Conversation
There was a problem hiding this comment.
Hi @davibarreira , I've added a few minor comments regarding code style. We are very picky with this given the size of the codebase. It helps keep things under control.
Besides these minor comments, I have another suggestion to improve the PR. Please check the animations in this post if you haven't already: https://observablehq.com/@meetamit/fibonacci-lattices
We need to implement the method for Box, Disk, Sphere, etc by relying on simple maps (x,y) -> f(x), g(y) that convert the Cartesian2D coords to LatLon or Cartesian3D.
Take for example the map with the disk (x, y) -> (2pix, sqrt(y)). You can plug these expressions into a Point(Polar(sqrt(y), 2pix)) to generate a Point with Polar coordinates. That way we don't need to convert the coordinates manually.
Another issue I encountered is the fact that your computation is assuming Float64. Sometimes users create geometries with Float32, so we need to preserve the machine type in the sampler. There is a function lentype(geom) that you can use to extrat the unitful length type from a geometry, and then numtype(lentype(geom)) will return either Float32 or Float64. If you are working with Cartesian coords, the lentype should be enough. You only need to go low-level with numtype if you need the machine type to use in non-length coordinates like angles in Polar or Spherical or LatLon coords.
src/sampling/fibonacci.jl
Outdated
| sample(Sphere((0,0,0), 1), FibonacciSampling(100)) | ||
| """ | ||
| struct FibonacciSampling <: SamplingMethod | ||
| size::Int |
There was a problem hiding this comment.
| size::Int | |
| n::Int |
to be consistent with the docstring and facilitate the lives of readers of the code in the future.
src/sampling/fibonacci.jl
Outdated
| end | ||
| end | ||
|
|
||
| function sample(::AbstractRNG, geom::Union{Sphere,Ball{𝔼{3}}}, method::FibonacciSampling) |
There was a problem hiding this comment.
We avoid dispatching on explicit Union as much as possible for code readability. Also, Sphere refers to the surface and Ball refers to the volume. Sphere == boundary(Ball). The sampling method should produce a different set of points for these two.
There was a problem hiding this comment.
Oh! I see. Did not know of the difference. I don't think there is a Fibonacci lattice method for solid balls. I'll remove the method for the 3D ball.
src/sampling/fibonacci.jl
Outdated
|
|
||
| function sample(::AbstractRNG, geom::Union{Sphere,Ball{𝔼{3}}}, method::FibonacciSampling) | ||
|
|
||
| # Compute x,y samples from regular sphere of radius 1 |
There was a problem hiding this comment.
| # Compute x,y samples from regular sphere of radius 1 | |
| # compute x,y samples from regular sphere of radius 1 |
comments in lowercase
src/sampling/fibonacci.jl
Outdated
| function sample(::AbstractRNG, geom::Union{Sphere,Ball{𝔼{3}}}, method::FibonacciSampling) | ||
|
|
||
| # Compute x,y samples from regular sphere of radius 1 | ||
| x, y, z = fibonnaci_sample_sphere(method.size) |
There was a problem hiding this comment.
we avoid underscores in function names and variables in our code style. sometimes we allow a leading underscore to signal an internal auxiliary function
src/sampling/fibonacci.jl
Outdated
| x = @. cos(theta) * r | ||
| y = @. sin(theta) * r | ||
|
|
||
| return x, y |
There was a problem hiding this comment.
| return x, y | |
| x, y |
no explicit return
src/sampling/fibonacci.jl
Outdated
| # Compute theta and phi | ||
| theta = 2 * pi * mod.(i / goldenRatio, 1) | ||
| r = @. √(i / n) | ||
| x = @. cos(theta) * r |
There was a problem hiding this comment.
prefer unicode when possible, in this case \theta
src/sampling/fibonacci.jl
Outdated
| """ | ||
| function fibonnaci_sample_disk(n::Int) | ||
| # Set the parameters | ||
| goldenRatio = (1 + sqrt(5)) / 2 |
src/sampling/fibonacci.jl
Outdated
| i = collect(0:(n - 1)) | ||
|
|
||
| # Compute theta and phi | ||
| theta = 2 * pi * mod.(i / goldenRatio, 1) |
…lter the ratio number. Also, sampling now preserves the lentype and numtype for the underlying geometry.
|
I've done a big refactor. I've tried making it look more like the HomogeneousSampler code. I also made the sampler more general by allowing the use of other ratios. One can use pi or other irrational numbers in order to get different samplings. I've also wrote a small section in the docs. |
Implementation for the Fibonacci Sampling method from #979 .
This PR implements the FibonacciSampling struct together with three methods for
samplewhich are used for spheres/ball, 3D disk and 2D ball.The current testing is not very thorough, having only a check if the samples fall within the geometry and whether the original sample point is properly centered.