Updated RANDOM_STATE_OR_SEED_LIKE to support np.random.Generator (#6531)#8184
Updated RANDOM_STATE_OR_SEED_LIKE to support np.random.Generator (#6531)#8184Prashik123 wants to merge 2 commits into
Conversation
arettig
left a comment
arettig
left a comment
There was a problem hiding this comment.
Hello, thanks for the PR. There are some things that need to be addressed here:
- The code does not pass multiple checks (
mypy,ruff,format-incremental). Please ensure these pass locally. - You state in the PR comment that you added tests but there are no tests added.
- You should not use the
closeskeyword in the comment since this PR is only part of the overall issue. - Your change allows passing a generator to
parse_random_statewhich will then be returned as a generator. Many places in the code assume the return of this function is aRandomState, creating numerous problems (which is whatmypyis catching).
You will need to rethink your design a little - take a look at the previous PR and discussion for this issue: #6566. There was some discussion about whether to extend RANDOM_STATE_OR_SEED_LIKE (like you did) or create a new type. From the looks of it, it was decided a new type was necessary.
| elif isinstance(random_state, int): | ||
| return np.random.RandomState(random_state) | ||
| elif isinstance(random_state, (np.random.RandomState, np.random.Generator)): | ||
| return random_state |
There was a problem hiding this comment.
this will return a np.random.Generator which will cause problems anywhere in Cirq that parse_random_state() is called
| """ | ||
| if random_state is None: | ||
| # FIX: Explicitly allow the np.random module to pass through | ||
| if random_state is None or random_state is np.random: |
There was a problem hiding this comment.
np.random is not an allowed type of random_state
|
@mhucka I think patching RANDOM_STATE_OR_SEED_LIKE is no longer desirable. I think creating a new path for generators while deprecating the random state one is more advisable. the RandomState class is effectively deprecated by numpy |
3 participants
Closes #6531
Description
This PR updates
cirq.RANDOM_STATE_OR_SEED_LIKEandcirq.value.parse_random_stateto explicitly supportnp.random.Generator.As discussed in the issue thread, this is Phase 1 of the implementation. It focuses strictly on the public normalization boundary and backward compatibility:
np.random.Generatoris now a valid input and passes through unmodified.int,np.random.RandomState,None, and the globalnp.randommodule remains fully backward compatible.ValueErrorspecifying the accepted types.random_state_test.pyto verifyGeneratorsupport and the updated error handling.Future PRs can now safely migrate internal call sites to prefer
Generatorfor parallel-safe behavior.Checklist
pytest).pylint) and type checks (mypy).Note for reviewers: Gemini 3.1 Pro Extended was used strictly as a conversational tutor to help me understand the repository structure and configure my local Windows testing environment. All code changes submitted in this PR are my own original creation in full compliance with the CLA.