Skip to content

Implement mkl_random.interfaces and update mkl_random#92

Open
ndgrigorian wants to merge 10 commits intomasterfrom
feature/add-numpy-random-interface
Open

Implement mkl_random.interfaces and update mkl_random#92
ndgrigorian wants to merge 10 commits intomasterfrom
feature/add-numpy-random-interface

Conversation

@ndgrigorian
Copy link
Collaborator

This PR adds mkl_random.interfaces and an interface numpy_random, aligning with the approach in mkl_fft to create drop-in replacements

In adding this, mkl_random main namespace was updated as follows

  • moved implementations of general functionality into a parent helper class _MKLRandomState
  • subclassed with MKLRandomState, which also implements all functionality exclusive to mkl_random (this way, numpy interface does not expose such functionality as multinormal_cholesky)
  • added subclass RandomState which effectively aliases MKLRandomState with a deprecation warning (as it will be removed from the namespace in a later release)
  • updated implementations of get_state, set_state, and multivariate_normal to align with changes in NumPy (especially the addition of keyword arguments in get_state and multivariate_normal)

@ndgrigorian ndgrigorian changed the title Implement mkl_random.interfaces and update mk_random Implement mkl_random.interfaces and update mkl_random Feb 25, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request implements a new mkl_random.interfaces module with a numpy_random interface that provides drop-in replacements for NumPy's legacy random functionality. The PR also refactors the main mkl_random namespace to improve code organization and align with recent NumPy API changes.

Changes:

  • Introduced a three-tier class hierarchy: _MKLRandomState (base), MKLRandomState (MKL-specific features), and RandomState (deprecated alias with warning)
  • Added mkl_random.interfaces.numpy_random module for NumPy compatibility
  • Updated get_state, set_state, and multivariate_normal methods to support new keyword arguments matching NumPy's current API

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pyproject.toml Updated package configuration to include the new mkl_random.interfaces subpackage
mkl_random/mklrand.pyx Refactored class hierarchy, moved methods between classes, added legacy parameter to get_state, added dict support to set_state, added check_valid and tol parameters to multivariate_normal, made poisson_lam_max a local variable
mkl_random/init.py Changed from wildcard import to explicit imports, added __all__ definition, imported interfaces module
mkl_random/interfaces/init.py New file that imports the numpy_random module
mkl_random/interfaces/numpy_random.py New public interface module that exports NumPy-compatible RandomState and functions
mkl_random/interfaces/_numpy_random.py New implementation module with RandomState class wrapping _MKLRandomState for NumPy compatibility
mkl_random/interfaces/README.md New documentation explaining the NumPy interface
mkl_random/tests/test_random.py Updated tests to use MKLRandomState instead of deprecated RandomState, improved test for multivariate_normal with new parameters, removed duplicate test function, improved code formatting
mkl_random/tests/test_regression.py Updated to use MKLRandomState instead of RandomState
.git-blame-ignore-revs Added commit hash for formatting changes to be ignored by git blame
Comments suppressed due to low confidence (5)

mkl_random/interfaces/_numpy_random.py:63

  • Typo in the docstring: "seed(seed=Nonee)" should be "seed(seed=None)".
        seed(seed=Nonee)

mkl_random/mklrand.pyx:5587

  • In the docstring, the parameter description says "If seed is None, then RandomState will try to read data from /dev/urandom...", but it should say "then MKLRandomState will try to read data" since this is the MKLRandomState class, not RandomState.
        If `seed` is ``None``, then `RandomState` will try to read data from
        ``/dev/urandom`` (or the Windows analogue) if available or seed from
        the clock otherwise.

mkl_random/mklrand.pyx:5038

  • The docstring for multivariate_normal is missing documentation for the newly added check_valid and tol parameters in the Parameters section. These parameters control validation behavior of the covariance matrix and should be documented to help users understand their purpose and valid values.
    def multivariate_normal(self, mean, cov, size=None, check_valid="warn", tol=1e-8):
        """
        multivariate_normal(mean, cov[, size, check_valid, tol])

        Draw random samples from a multivariate normal distribution.

mkl_random/mklrand.pyx:1111

  • The new legacy parameter for get_state is not tested. Consider adding tests to verify that get_state(legacy=False) returns a dictionary with the expected structure and that get_state(legacy=True) returns the traditional tuple format.
    def get_state(self, legacy=True):
        """
        get_state()

mkl_random/mklrand.pyx:1266

  • The __reduce__ method in _MKLRandomState still references __RandomState_ctor, but this will cause incorrect unpickling behavior for MKLRandomState instances. When a MKLRandomState object is pickled and then unpickled, it will be reconstructed as a RandomState object instead, which will trigger an unnecessary deprecation warning. The MKLRandomState class should override this method to reference __MKLRandomState_ctor instead.
    def __reduce__(self):
        global __RandomState_ctor
        return (__RandomState_ctor, (), self.get_state())

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

mkl_random/interfaces/_numpy_random.py:63

  • Typo in docstring: "seed=Nonee" should be "seed=None"
        seed(seed=Nonee)

mkl_random/interfaces/_numpy_random.py:226

  • Unnecessary trailing comma after the last argument in the function call. This is inconsistent with the style used in other similar calls in this file. While not a syntax error, it should be removed for consistency.
        return super().beta(a=a, b=b, size=size,)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

mkl_random/mklrand.pyx:5999

  • The __RandomState_ctor function will trigger a deprecation warning every time a pickled RandomState object is unpickled, because it calls RandomState(seed=0) which has the deprecation warning in its __init__. This creates unwanted warning spam when loading pickled objects. Consider using warnings.catch_warnings() to suppress the deprecation warning within this function, or construct the object in a way that bypasses __init__.
def __RandomState_ctor():
    """
    Return a RandomState instance.
    This function exists solely to assist (un)pickling.
    Note that the state of the RandomState returned here is irrelevant, as this function's
    entire purpose is to return a newly allocated RandomState whose state pickle can set.
    Consequently the RandomState returned by this function is a freshly allocated copy
    with a seed=0.
    See https://github.com/numpy/numpy/issues/4763 for a detailed discussion
    """
    return RandomState(seed=0)

mkl_random/interfaces/README.md:18

  • The documentation lists all distributions but is missing logseries, which is actually available in the interface (as shown in the __all__ list at lines 70 and 121 of numpy_random.py). Add logseries to the list of distributions in the README.
* distributions: `beta`, `binomial`, `chisquare`, `dirichlet`, `exponential`, `f`, `gamma`, `geometric`, `gumbel`, `hypergeometric`, `laplace`, `logistic`, `lognormal`, `multinomial`, `multivariate_normal`, `negative_binomial`, `noncentral_chisquare`, `noncentral_f`, `normal`, `pareto`, `poisson`, `power`, `rayleigh`, `standard_cauchy`, `standard_exponential`, `standard_gamma`, `standard_normal`, `standard_t`, `triangular`, `uniform`, `vonmises`, `wald`, `weibull`, and `zipf`.

mkl_random/mklrand.pyx:1110

  • The docstring signature for get_state shows get_state() with no parameters, but the method now accepts a legacy parameter with a default value. Update the docstring to show get_state(legacy=True) to match the actual signature.
        get_state()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

mkl_random/mklrand.pyx:5054

  • The multivariate_normal method signature now includes check_valid and tol parameters, but these are not documented in the Parameters section of the docstring. These parameters should be documented to inform users about the new covariance validation options. Based on the implementation, check_valid can be 'warn', 'raise', or 'ignore' and controls covariance matrix validation, while tol specifies the tolerance for the positive-semidefinite check.
    def multivariate_normal(self, mean, cov, size=None, check_valid="warn", tol=1e-8):
        """
        multivariate_normal(mean, cov[, size, check_valid, tol])

        Draw random samples from a multivariate normal distribution.

        The multivariate normal, multinormal or Gaussian distribution is a
        generalization of the one-dimensional normal distribution to higher
        dimensions.  Such a distribution is specified by its mean and
        covariance matrix.  These parameters are analogous to the mean
        (average or "center") and variance (standard deviation, or "width,"
        squared) of the one-dimensional normal distribution.

        Parameters
        ----------
        mean : 1-D array_like, of length N
            Mean of the N-dimensional distribution.
        cov : 2-D array_like, of shape (N, N)
            Covariance matrix of the distribution. It must be symmetric and
            positive-semidefinite for proper sampling.
        size : int or tuple of ints, optional
            Given a shape of, for example, ``(m,n,k)``, ``m*n*k`` samples are
            generated, and packed in an `m`-by-`n`-by-`k` arrangement.  Because
            each sample is `N`-dimensional, the output shape is ``(m,n,k,N)``.
            If no shape is specified, a single (`N`-D) sample is returned.

mkl_random/tests/test_random.py:689

  • There is an extra space before method= in this line. The spacing should be consistent with the rest of the file where a single space is used after a comma (e.g., lines 832, 841).
        mean=.123456789, sigma=2.0, size=(3, 2),  method='Box-Muller2'

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (6)

mkl_random/mklrand.pyx:6002

  • __RandomState_ctor is used for unpickling, but it constructs RandomState(seed=0), which will emit the deprecation warning in RandomState.__init__. Consider using a private/no-warning ctor for unpickling so loading pickles doesn’t unexpectedly warn.
    with a seed=0.
    See https://github.com/numpy/numpy/issues/4763 for a detailed discussion
    """
    return RandomState(seed=0)

mkl_random/mklrand.pyx:1166

  • New API surface: get_state(legacy=False) returns a dict format intended to be consumable by set_state, but there are no tests covering the dict round-trip. Please add a regression test that does set_state(get_state(legacy=False)) to prevent accidental breakage.
            return {
                "bit_generator": brng_name,
                "state": {
                    "mkl_stream": bytestring
                    }
                }

mkl_random/init.py:96

  • mkl_random/__init__.py eagerly imports mkl_random.interfaces, which pulls in the full NumPy-compat interface on every import mkl_random. This is redundant (submodules are importable on demand) and adds import-time overhead/side effects; consider removing the eager import or making it lazy.
import mkl_random.interfaces

mkl_random/mklrand.pyx:1184

  • set_state now accepts dict-form state as well as tuple/list, but this docstring section still describes only the tuple layout. Please update the parameter docs to also describe the accepted dict format (or explicitly document both).
        state : {tuple(str, bytes), dict}
            The `state` tuple has the following items:

            1. a string specifying the basic psedo-random number generation
               algorithm.
            2. a bytes object holding content of Intel MKL's stream for the

mkl_random/mklrand.pyx:1127

  • get_state can now return either a legacy tuple or a dict (when legacy=False), but this docstring still describes the output as “the returned tuple …”. Please update the return-value docs to accurately describe both possible shapes.
        out : {tuple(str, bytes), dict}
            The returned tuple has the following items:

            1. a string specifying the basic psedo-random number generation
               algorithm.

mkl_random/mklrand.pyx:1231

  • This error message says the argument “must be a list of 2 elements”, but set_state also accepts tuples (and dicts via the other branch). Consider changing the message to reflect the actual accepted types/shape (e.g., tuple/list of length 2).
                raise ValueError("The argument to set_state must be a list of 2 elements")
        elif isinstance(state, dict):

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Slips in changes updating get_state, set_state, and multivariate_normal to align with recent numpy changes
now ignore irrelevant RuntimeWarnings and align with the test in NumPy's test suite
cleans up visual indentation and various linter/style mistakes
__reduce__ methods were not added in subclasses, only in superclass (_MKLRandomState), so the proper constructors would never be called, thus pickling would produce different objects
not actually necessary per linters
@ndgrigorian ndgrigorian force-pushed the feature/add-numpy-random-interface branch from e612350 to 5ffbb5a Compare February 27, 2026 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants