-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
TST: add more thorough SPEC 7 tests #21948
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
Conversation
The So I intend to make a commit with the changes as @mdhaber suggests. Just wanted to let folks know so this doesn't get merged first and so people can respond here if this is not desired. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have completed my review/changes based on sparse.random. After playing around a little more I decided to stick with @mdhaber's approach for handling rng=None
and random_state=None
in random_array
. This is the smoothest way forward IMO.
I did change the test calls .todense()
to .toarray()
to avoid use of the older method name (that returns a np.matrix/np.array depending on sparse matrix/array type).
I also rebased on main to hopefully get more green checks.
The failing lint test seems to be related to It is reporting some methods "which does not start with 'test'" but it is not reporting all such functions and I see no pattern. I check the main branch and it reports one such file. So it is not specific to this branch. |
Was it failing in the PR originally? (Can't tell after the force push - please consider merge rather than rebase + force push when working on my branch. Commit history is only important at the end, and I think will be squash-merged anyway.) If not, maybe we should just re-run CI? |
Sorry about the rebase on your branch @mdhaber . Didn't realize the side effects of that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor query but otherwise looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks both!
Reference issue
Closes gh-21833
(I'm not aware of anything else that must be transitioned, and I think gh-21921 can be considered separate.)
What does this implement/fix?
The
_transition_to_rng
decorator was helpful, but not all our cases matched the usual pattern, so I thought it would be helpful to add a standard test for all the SPEC 7 transition work.During the process, I found two small issues:
scipy.sparse.random_array
documentation has always stated that it uses theRandomState
singleton if ifrandom_state
is not provided, but it actually uses an unseededGenerator
(for speed). I have opted to leave the documentation alone and change the implementation; this adds a small incentive to transition to the newrng
argument. @dschult, if you don't want to go this route, please add a commit to this branch: undo the change torandom_array
, passreplace_doc=False
into its_transition_to_rng
decorator factory, manually adjust the documentation, and change the tests as necessary.multivariate_normal_qmc
andmultinomial_qmc
receivedRandomState
via therandom_state
keyword, we were getting an error when this was passed along toSobol
via therng
keyword. The simple fix is to pass them to theSobol
via therandom_state
keyword, instead. Note that I'm only using therandom_state
keyword if the user has already done so, so I think that's OK; if we get duplicateDeprecationWarning
s when those get turned on, we can deal with it then. (We're branching soon.)I also found a small issue with the adjustments to
PermutationMethod
andBootstrapMethod
before adding these tests. The gist of it is that we need to keep track of whether the user passed in the argument asrandom_state
orrng
so that the call topermutation_test
orbootstrap
does the right thing with it. For simplicity, we also probably shouldn't allowrng
to be mutated during the transition. It's just an extra thing to worry about, so I removed the setter.