Skip to content

Conversation

mdhaber
Copy link
Contributor

@mdhaber mdhaber commented Nov 26, 2024

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:

  • The scipy.sparse.random_array documentation has always stated that it uses the RandomState singleton if if random_state is not provided, but it actually uses an unseeded Generator (for speed). I have opted to leave the documentation alone and change the implementation; this adds a small incentive to transition to the new rng argument. @dschult, if you don't want to go this route, please add a commit to this branch: undo the change to random_array, pass replace_doc=False into its _transition_to_rng decorator factory, manually adjust the documentation, and change the tests as necessary.
  • When multivariate_normal_qmc and multinomial_qmc received RandomState via the random_state keyword, we were getting an error when this was passed along to Sobol via the rng keyword. The simple fix is to pass them to the Sobol via the random_state keyword, instead. Note that I'm only using the random_state keyword if the user has already done so, so I think that's OK; if we get duplicate DeprecationWarnings 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 and BootstrapMethod before adding these tests. The gist of it is that we need to keep track of whether the user passed in the argument as random_state or rng so that the call to permutation_test or bootstrap does the right thing with it. For simplicity, we also probably shouldn't allow rng to be mutated during the transition. It's just an extra thing to worry about, so I removed the setter.

@mdhaber mdhaber added the maintenance Items related to regular maintenance tasks label Nov 26, 2024
@j-bowhay j-bowhay added this to the 1.15.0 milestone Nov 28, 2024
@dschult
Copy link
Contributor

dschult commented Nov 29, 2024

The random_array choice of default being default_rng() was intentional and I think we should keep it that way. The documentation doesn't match this, and after our discussions regarding the default, I appear to have updated the behavior without updating the docs.

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.

Copy link
Contributor

@dschult dschult left a 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.

@dschult
Copy link
Contributor

dschult commented Dec 1, 2024

The failing lint test seems to be related to tools/check_test_name.py which is called by dev.py after lint.py finishes.

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.

@mdhaber
Copy link
Contributor Author

mdhaber commented Dec 1, 2024

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?

@dschult
Copy link
Contributor

dschult commented Dec 2, 2024

Sorry about the rebase on your branch @mdhaber . Didn't realize the side effects of that.
I've reset the branch to the 3 original commits plus my 1 tweak commit. No rebase or merge.
The lint error is present. But it is not reported in the lint test after the 3rd commit -- I think because those lint tests failed due to UP031, so it never continued on to check_test_names.py.
I'm not sure what is going on, but still investigating.

Copy link
Member

@j-bowhay j-bowhay left a 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

Copy link
Member

@j-bowhay j-bowhay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks both!

@j-bowhay j-bowhay merged commit 82ee025 into scipy:main Dec 2, 2024
37 checks passed
@mdhaber
Copy link
Contributor Author

mdhaber commented Dec 2, 2024

Thanks for fixing that @dschult and thanks for the review @j-bowhay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks scipy._lib scipy.sparse scipy.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SPEC 7 Transition Tracker
3 participants