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

Now random stuff can receive, and use, a RandomState object #1348

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ricardodeazambuja
Copy link

PROS:

  • you can pass a numpy.random.RandomState object to everything that uses a pseudo-random generator
  • you don't always need to pass a numpy.random.RandomState object because, by default, it will fall back to using the current state of numpy.random (if you don't care about reproducibility or need to debug something, it's just fine)
  • pickle still works 😄

CONS:

  • Python standard library random was totally replaced by numpy.random (sometimes adapted), therefore it may be slower in some situations and it may need some TLC to optimize things
  • Old code that was using np.seed or random.seed will be even more unreliable now
  • Numpy is already moving away from RandomState... something to think in the near future
  • Tests using the files from tests/files were skipped
  • Not deeply debugged or tested besides the automatic tests

Standard library random replaced by numpy.random (sometimes adapted).
@Dipet
Copy link
Collaborator

Dipet commented Nov 24, 2022

Current implementation will work incorrectly with python multiprocessing because each process wil use the same random seed.
For this reason we used random istead of np.random as base random generator.

@Dipet
Copy link
Collaborator

Dipet commented Nov 24, 2022

@ricardodeazambuja
Copy link
Author

Hi @Dipet, I just pushed a simple modification that has a test specifically for the situation where the user wants to use the random state. Could you please give me another code example where this implementation will work incorrectly with python multiprocessing since it passes the original test? I never used albumentations before and I will start using it just because it seemed easier to implement the use of random states compared to torchvision transforms 😃
Thanks!

@ricardodeazambuja
Copy link
Author

I've been using this modified version passing a Numpy RandomState object and it is super useful for reproducibility. Albumentations is amazing, and I love using it, so I am here offering my help. What else should I do to get it merged?

@jpcbertoldo
Copy link

Why was this closed?

@ricardodeazambuja
Copy link
Author

@jpcbertoldo, this PR is still alive and kicking 😄
#1377 was closed because it depends on this one, but this one never gets pulled and I need to keep updating it in the hope one day it will get accepted 😢

@jpcbertoldo
Copy link

Ah yes, I confused with #1377
my bad hehe

hope one day it will get accepted

inshallah! it's a major upgrade

For this reason we used random istead of np.random as base random generator.

Would it be possible to use random.Random instead of np.random.RandomState?

@jpcbertoldo
Copy link

jpcbertoldo commented Jan 31, 2023

np.random

Do you know if there's any issue open in numpy for this issue?

I'm not sure because I don't know that much about parallelization, but could there be any hint in here?

https://numpy.org/doc/stable/reference/random/parallel.html

@ricardodeazambuja
Copy link
Author

ricardodeazambuja commented Jan 31, 2023

np.random

Do you know if there's any issue open in numpy for this issue?

I'm not sure because I don't know that much about parallelization, but could there be any hint in here?

https://numpy.org/doc/stable/reference/random/parallel.html

Everything is working, and I just merged the latest stuff from master, again. The problem was in the test itself because the test was not expecting something using a random state, but I fixed it long time ago.

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.

None yet

3 participants