search

pvigier / perlin-numpy

A fast and simple perlin noise generator using numpy
https://pvigier.github.io/2018/06/13/perlin-noise-numpy.html
MIT License
304 stars 50 forks source link

Random Seed as Parameter & Added Res Validity Check #7

Open AyrtonB opened 3 years ago

AyrtonB commented 3 years ago

This PR lets the user set a random seed value for both the 2d and 3d perlin noise functions

pvigier commented 3 years ago

Hi,

Do we really want to set the numpy seed inside the functions? It may silently mess the global state of applications that consume the library and provoke strange bugs.

AyrtonB commented 3 years ago

Hi @pvigier,

That's a really good point for the case when people don't want control over the seed.

This could be handled by including a if seed is not None and only running np.random.seed(seed) if that's the case.

Permafacture commented 9 months ago

@AyrtonB I've implemented seed setting the way numpy developers recommend now in #15

jaroslavknotek commented 5 months ago

Hello, I was about to submit PR with the same goal as this one but with different implementation.

In my project, I need to have even better control over the random state to the extend that it is not possible to change global seed np.random.seed at all. Instead, I need to pass random state (np.random.RandomState).

Additionally, @Permafacture says in #15 that the developers recommendation were implemented but I see here that devs themselves recommend to use RandomState as well.

Do you think that making PR with Random State would make it into master or should I just make my own fork?

Permafacture commented 5 months ago

@jaroslavknotek I might be misunderstanding. I wrote a PR that that has not been accepted to use the best practice the numpy developers intend for people to use. I did not say the current main branch had implemented random numbers correctly. Are you advocating for my PR to be accepted or are you saying you'd start a new PR and if so how would it be different?

jaroslavknotek commented 5 months ago

Sorry for misunderstanding. My point is that on one hand I am in-fact advocating for better control over using random seeds but on the other hand I don't think that your implementation improved the situation much. To support that I added a link to numpy developers who advise to use np.random.RandomState instead of np.random.seed.

The use of the latter, regardless of whether used outside of noise function (as original implementation) or inside (your implementation), is not suitable as it changes a global behavior of numpy random number generator. In a situation where you use separate libraries that need to create random number, you need both to generate it without affecting the other.

More specifically, using perlin noise function that do np.random.seed will 'reset' the behavior for all subsequent callers of np.random.rand callers. To avoid this behavior, any function using random numbers should have its own generator to avoid influencing others. This is very important if you want to have "random" yet still reproducible results.

Permafacture commented 5 months ago

I don't think you understand my changes. rng = np.random.default_rng(seed) creates a local random number generator separate from the global one. No where am I setting the global random.seed as you're saying. Creating a local random number generator with a seed here is the best practice put forward by the numpy developers. If you disagree, can you comment on the pull request where you think it is incorrect?

jaroslavknotek commented 5 months ago

I am sorry for the confusion. I was looking into changed files all the time and didn't look into #15 where the changes are indeed correctly implemented. The points I made above are in relation to the current PR #7 and are therefore obsolete.