lazarusA / CoherentNoise.jl

A comprehensive suite of coherent noise algorithms and composable tools for manipulating them.
https://lazarusa.github.io/CoherentNoise.jl/stable/
MIT License
67 stars 6 forks source link

Consider passing RNG instead of seed to samplers #7

Closed juliohm closed 2 years ago

juliohm commented 2 years ago

First of all, congratulations on this great package! :tada: I work a lot with generation of random images in geostatistics (GeoStats.jl) and it is nice to see more algorithms available.

May I suggest sampler_type(rng=...) instead of sampler_type(seed=...)? In Julia, whenever we want to guarantee reproducibility with random numbers we pass the generator object instead of the seed, and it goes as the first positional argument instead of a keyword argument. See the rand docstring for example.

mfiano commented 2 years ago

A lot of thought went into this idea early on in the design process, and was ultimately rejected, as it poses a few problems.

Each sampler requires its own distinct RNG, and in a pipeline of modifiers, there can be no shared state if one wants reproducible results and the ability to inspect a sub-graph of samplers.

Not all RNGs allow seeding, such as RandomDevice, and is not required for AbstractRNG derivations.

The Random interface does not have provisions for retrieving the seed a RNG was initialized with, which the current implementation relies on. We do this by wrapping the RNG in a type that also stores the seed, and to do this generically for any concretely-typed RNG would require a parametric struct with parameters that would propagate through the implementation, perhaps even into the user API.

Some RNGs place restrictions on the type of the seed they are initialized with. For example, the default Xorshiro256++ RNG in Random cannot be seeded with Signed integers, as that is a DomainError.

One could argue that the "seed" is a user-centric API feature of CoherentNoise.jl, and the RNG is an implementation detail that is encapsulated and free to change in the future.

Changing this would be a breaking change and for more disadvantages than the advantage of consistency with Julia conventions.

I will think about all of this for a future version though. Thank you for your kind words and suggestion!

mfiano commented 2 years ago

Ok, so first of all, thank you very much for the suggestion. I have thought about this and I decided that it doesn't make a lot of sense to pass a RNG to a sampler. This is because "sampler" is a bad name, and will be changed in the future. CoherentNoise.jl samplers are nothing like their more random PRNG cousins, and the random number generator is an implementation detail. Infact, we rely on a specific RNG baked into the library for reproducibility (Julia is free to change the results of its RNGs even in patch-level releases). In short, the RNG that CoherentNoise.jl uses should not be exposed to the user, and the user should not be able to specify a custom RNG, as "samplers" are more like "evaluators", and the randomosity is just an extra feature baked in deep and would require significant changes for no real gain.

However, there are two things that we can do, one of which I implemented in the development version (not an official release yet):

1) The seed parameter of a sampler now defaults to nothing, which correlates to using your machine's hardware random number generator to produce a seed automatically. This means each sampler instance will have non-deterministic output values when sampled with the same set of coordinates. This makes more sense than the old method of defaulting seed to 0, as it makes it convenient for the user to zero-in on a noise result they like, and then query the seed of the sampler to reproduce those results at a later time if they desire.

2) You can make use of the modifier interface. A modifier is capable of modifying the input coordinates before sampling, or the output value returned from sampling. In this case, you could easily write a modifier (which is just another sampler). This has to be done carefully though, as you don't want to just randomize the input coordinates, otherwise the result would not be spatially coherent. You would need to do something like the cache modifier, which would cache a set of random values to add to input coordinates. I don't really like this idea, because the library doesn't really have first-class support for caching sampler state.

I think option 1, which is already implemented, should cover 99% of what you're after, without exposing the implementation details of the library. I'm going to close this issue as solved, but feel free to discuss this further.

juliohm commented 2 years ago

Regarding reproducible random numbers there is a package for that which is independent of Julia versions. Please double check before reinventing the wheel. Search for reproducible random in JuliaHub.

Em dom., 28 de ago. de 2022 14:27, Michael Fiano @.***> escreveu:

Closed #7 https://github.com/mfiano/CoherentNoise.jl/issues/7 as completed.

— Reply to this email directly, view it on GitHub https://github.com/mfiano/CoherentNoise.jl/issues/7#event-7273774955, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZQW3NE7OHQSQ6FODNKOQDV3OOOTANCNFSM57LKRMQA . You are receiving this because you authored the thread.Message ID: @.***>

mfiano commented 2 years ago

We already use one of the two libraries. StableRNGs.jl and RandomNumbers.jl are by the same organization, and we chose the latter, as LCG provided by the former is not the best RNG.