MAINT: stats.qmc: transition to rng (SPEC 7) #21931
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mdhaber
commented
Nov 23, 2024
scipy/stats/_qmc.py
Outdated
| As part of the `SPEC-007 <https://scientific-python.org/specs/spec-0007/>`_ | ||
| transition from use of `numpy.random.RandomState` to | ||
| `numpy.random.Generator`, this keyword was changed from `seed` to | ||
| `rng`. For an interim period, both keywords will continue to work, although |
There was a problem hiding this comment.
This is the source of the lint failure. I'll fix it when we have a reviewer for this. Reviewer: mind if I dedent this whole docstring? The whole thing one level higher than necessary.
tupui
approved these changes
Nov 23, 2024
|
thanks both! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reference issue
Toward gh-21833
What does this implement/fix?
To prepare for the deprecation and removal of
seedand its legacy behavior fromstats.qmcclasses., this allows for use of the new keywordrngalongsideseed.Additional information
Note that when the
_transition_to_rngdecorator receives an integer as therngargument, it normalizes it usingdefault_rngbefore passing it to the wrapped function. Also, if aQMCEngineis passed anp.random.default_rngdirectly, it doesn't use it; rather, it spawns a new RNG from it. Therefore, with the decorator in place, both of the following happen in sequence: therngargument is always normalized, and a new Generator is spawned from it.So as-written, if we want to preserve the stream of numbers when the
_transition_to_rngdecorator is removed, we would always need to normalize the argumentrngargument withdefault_rngAND spawn a new Generator from that; we would not keep the current logic that "if the argument is an instance ofGenerator, spawn; otherwise, normalize".There is no problem with this; it's just a bit peculiar. A motivated individual is welcome to adjust the current implementation after this PR merges if this is not the desired long-term behavior.