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

consistent usages of np.RandomState in the code #6888

Open
wyli opened this issue Aug 18, 2023 · 2 comments
Open

consistent usages of np.RandomState in the code #6888

wyli opened this issue Aug 18, 2023 · 2 comments
Labels

Comments

@wyli
Copy link
Contributor

wyli commented Aug 18, 2023

  1. There are some usages of np.random.* in the transforms (see below). ..

./monai/transforms/signal/array.py:276:np.random.choice
./monai/transforms/signal/array.py:357:np.random.choice
./monai/transforms/spatial/array.py:1214:np.random.randint
./monai/transforms/spatial/dictionary.py:2347:np.random.randint
./monai/transforms/spatial/dictionary.py:698:np.random.randint
./monai/transforms/utils.py:1220:np.random.random
./monai/transforms/utils.py:246:np.random.randint
./monai/transforms/utils.py:510:np.random.randint
./monai/transforms/utils.py:552:np.random.random
./monai/transforms/utils.py:607:np.random.random

  1. There are some usages in other parts of the main code. Same question on what to do with those remains:

./monai/apps/deepedit/interaction.py:65:np.random.choice
./monai/apps/deepedit/transforms.py:159:np.random.choice
./monai/apps/deepedit/transforms.py:60:np.random.choice
./monai/apps/detection/transforms/dictionary.py:1305:np.random.randint
./monai/apps/nuclick/transforms.py:370:np.random.choice
./monai/apps/nuclick/transforms.py:377:np.random.choice
./monai/auto3dseg/analyzer.py:190:np.random.rand
./monai/auto3dseg/analyzer.py:191:np.random.rand
./monai/auto3dseg/analyzer.py:292:np.random.rand
./monai/auto3dseg/analyzer.py:374:np.random.rand
./monai/auto3dseg/analyzer.py:862:np.random.rand
./monai/auto3dseg/operations.py:119:np.random.rand
./monai/auto3dseg/operations.py:120:np.random.rand
./monai/auto3dseg/operations.py:121:np.random.rand
./monai/auto3dseg/operations.py:122:np.random.rand
./monai/auto3dseg/operations.py:62:np.random.rand
./monai/data/synthetic.py:142:np.random.random
./monai/data/synthetic.py:65:np.random.random
./monai/data/utils.py:125:np.random.randint
./monai/utils/misc.py:353:np.random.seed
./monai/visualize/utils.py:92:np.random.rand
./monai/visualize/utils.py:97:np.random.rand

  1. What to do with usage of np.random.* in tests/**/*.py files? Numpy states that:

New code should use the normal method of a Generator instance instead; please see the Quick Start.

So it seems like there is no need to migrate those immediately, but just wanted to mention that here. Since there are around ~390 usages in tests I am not listing them here.

I found usages using the following bash command: find . -name "*.py" -exec grep -o -H -n -E 'np.random.\w+' {} \; | grep -v "RandomState" | sort | uniq

Originally posted by @john-zielke-snkeos in #6854 (comment)

@wyli
Copy link
Contributor Author

wyli commented Aug 18, 2023

(cc @ericspod @john-zielke-snkeos follow-up of the discussions in #6854, self.R and RandomState is currently preferred over np.random.*, this ticket is to track the issue.
e.g. preferred usage:

rs: np.random.RandomState = np.random.random.__self__ if random_state is None else random_state # type: ignore
)

@ericspod
Copy link
Member

If we wanted to first change the usages of np.random.* to use a RandomState then migrate in the future to Generator we could do the following:

  • Change the usage of np.random.* in transforms to use self.R. This will change some expected deterministic behaviour but would be more reproducible.
  • In other places in core change routines to accept a RandomState object as an argument and use that, defaulting to np.random if none is provided. This would cover most use cases with minimal addition. @wyli 's code snippet is probably the technique to use for this.
  • In apps/* and auto3dseg ask contributors what they would want to do, mentioning we're trying to tighten up the implementation with an eye to future migration.

We do need to have a more considered discussion about what to do about migration at some point, either we move to Generator or we do implement our own class to replace RandomState usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants