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

ENH: check_random_state: Accept np.random.Generator #654

Merged
merged 9 commits into from
Nov 27, 2022

Conversation

oyamad
Copy link
Member

@oyamad oyamad commented Nov 23, 2022

Numpy's recommended procedure to generate random numbers now seems to be to use np.random.Generator instead of np.random.RandomState: https://numpy.org/doc/stable/reference/random/index.html

This PR is to update check_random_state to accept np.random.Generator, following the corresponding code in scipy:
https://github.com/scipy/scipy/blob/b4a970bf4eba1065ea31f8d0988cd4ec523a07d4/scipy/_lib/_util.py#L174-L203

There are discussions on this at scipy and scikit-learn:

The issue is that some methods are present for np.random.RandomState but not for np.random.Generator: see the table in here. Thus, for example, random_sample() and rand() have to be replaced with random(), and randn() with standard_normal() and so on.
These can be done as commits added to this PR or as independent PRs.

TODOs:

  • Replace np.random.RandomState specific methods to np.random.Generator compatible methods
    • random_sample() -> random()
    • rand() -> random(), but rand(d0, d1, ..., dn) -> random((d0, d1, ..., dn)) (or random(size=(d0, d1, ..., dn)))
    • randn() -> standard_normal(), but randn(d0, d1, ..., dn) -> standard_normal((d0, d1, ..., dn)) (or standard_normal(size=(d0, d1, ..., dn)))
    • randint() -> util.rng_integers()
  • Update docstrings
  • Add tests

@coveralls
Copy link

coveralls commented Nov 23, 2022

Coverage Status

Coverage increased (+0.05%) to 95.235% when pulling 46b2bd3 on np-random-generator into c20ea25 on master.

@mmcky mmcky added this to the v0.6.0 milestone Nov 23, 2022
@oyamad
Copy link
Member Author

oyamad commented Nov 23, 2022

There is incompatibility between RandomState.randint and Generator.integers (i.e., there is no method with a common name).

Use util.rng_integers, which is just an alias of scipy._lib._util.rng_integers.

@oyamad
Copy link
Member Author

oyamad commented Nov 23, 2022

As demonstrated by #655, some calls can be replaced independent of this PR, without any breaking of the code (except for randint; see above).

@oyamad oyamad force-pushed the np-random-generator branch from 730582d to ec511d2 Compare November 24, 2022 09:00
Comment on lines 87 to 88
return scipy_rng_integers(gen, low, high=high, size=size,
dtype=dtype, endpoint=endpoint)
Copy link
Member Author

Choose a reason for hiding this comment

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

I would say this is redundant. Isn't just importing scipy._lib._util.rng_integers directly enough?

@oyamad oyamad force-pushed the np-random-generator branch from e36d515 to 6f97ffc Compare November 26, 2022 04:14
@oyamad
Copy link
Member Author

oyamad commented Nov 26, 2022

I propose to update the docstrings as follows (to keep them short):

def some_function(main_arguments, random_state=None):
    """
    Parameters
    ----------
    ...

    random_state : int or np.random.RandomState/Generator, optional
        Random seed (integer) or np.random.RandomState or Generator
        instance to set the initial state of the random number generator
        for reproducibility. If None, a randomly initialized RandomState
        is used.

    """
    ...
class SomeClass:
    def some_method(main_arguments, random_state=None):
        """
        Parameters
        ----------
        ...

        random_state : int or np.random.RandomState/Generator, optional
            Random seed (integer) or np.random.RandomState or Generator
            instance to set the initial state of the random number
            generator for reproducibility. If None, a randomly
            initialized RandomState is used.

        """
        ...

@oyamad oyamad force-pushed the np-random-generator branch from 75a1c43 to 2511646 Compare November 27, 2022 06:41
@oyamad oyamad changed the title [WIP] ENH: check_random_state: Accept np.random.Generator ENH: check_random_state: Accept np.random.Generator Nov 27, 2022
@oyamad oyamad added ready and removed in-work labels Nov 27, 2022
@oyamad
Copy link
Member Author

oyamad commented Nov 27, 2022

Let me merge this. Thanks for your help @Smit-create.

@oyamad oyamad merged commit 366c558 into master Nov 27, 2022
@oyamad oyamad deleted the np-random-generator branch November 27, 2022 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants