fix pr comments and suggestions

Author: vchamarthi

CHANGELOG.md

@@ -7,12 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 # [dev] (MM/DD/YYYY)
 
 ### Added
-* Added tests for the array-valued parameter paths of the location and scale distributions [gh-171](https://github.com/IntelPython/mkl_random/pull/171)
 * Added support for `array_like` (broadcastable) `low`/`high` bounds in `randint` [gh-168](https://github.com/IntelPython/mkl_random/pull/168)
 
 ### Changed
-* Sped up `normal`, `uniform`, `exponential`, `laplace`, `gumbel`, `logistic`, `rayleigh` and `lognormal` for array-valued parameters [gh-171](https://github.com/IntelPython/mkl_random/pull/171)
-* The random streams for the array-valued-parameter paths of the distributions above have changed: with a fixed seed these now produce different (but equally valid) samples. Scalar-parameter paths are unaffected. [gh-171](https://github.com/IntelPython/mkl_random/pull/171)
+* Sped up `normal`, `uniform`, `exponential`, `laplace`, `gumbel`, `logistic`, `rayleigh` and `lognormal` for array-valued parameters. Seeded results change for these array paths; scalar paths are unchanged [gh-171](https://github.com/IntelPython/mkl_random/pull/171)
+* Array parameters for these distributions must broadcast to the requested `size` without adding dimensions; previously accepted mismatches now raise `ValueError` [gh-171](https://github.com/IntelPython/mkl_random/pull/171)
+* `uniform` with array-valued bounds may return `high` due to floating-point rounding [gh-171](https://github.com/IntelPython/mkl_random/pull/171)
 * Pinned Cython in the Coverity Scan workflow so generated code stays stable between scans, and added `coverity/README.md` documenting the known Cython-boilerplate false positives and the scan review checklist [gh-164](https://github.com/IntelPython/mkl_random/pull/164)
 
 ### Fixed

mkl_random/mklrand.pyx

@@ -887,7 +887,8 @@ cdef object vec_lognormal_array(
     """Draw lognormals with array-valued parameters, one call per request.
 
     Uses the normal fill: the parameters sit inside the exponential, so no
-    affine step applies to a standardised lognormal, but exp(mean + sigma * z) does.
+    affine step applies to a standardised lognormal,
+    but exp(mean + sigma * z) does.
     """
     cdef object array
 
@@ -2840,16 +2841,19 @@ cdef class _MKLRandomState:
         Samples are uniformly distributed over the half-open interval
         ``[low, high)`` (includes low, but excludes high).  In other words,
         any value within the given interval is equally likely to be drawn
-        by `uniform`.
+        by `uniform`. With array-valued bounds, floating-point rounding
+        may include the upper boundary in the returned samples.
 
         Parameters
         ----------
         low : float, optional
             Lower boundary of the output interval.  All values generated will be
             greater than or equal to low.  The default value is 0.
         high : float
-            Upper boundary of the output interval.  All values generated will be
-            less than high.  The default value is 1.0.
+            Upper boundary of the output interval. With array-valued bounds,
+            high may be included due to floating-point rounding in
+            ``low + (high - low) * U``, where ``U`` is drawn from ``[0, 1)``.
+            The default value is 1.0.
         size : int or tuple of ints, optional
             Output shape.  If the given shape is, e.g., ``(m, n, k)``, then
             ``m * n * k`` samples are drawn.  Default is None, in which case a

mkl_random/tests/test_random.py

@@ -1327,7 +1327,7 @@ def test_two_param_array_matches_scalar(name, draw, pa, pb):
     np.testing.assert_allclose(
         arrayed,
         scalar,
-        rtol=1e-9,
+        rtol=1e-8 if name == "lognormal" else 1e-9,
         atol=1e-9 * float(np.std(scalar)),
         err_msg=f"{name}: array-parameter path disagrees with scalar path",
     )
@@ -1337,14 +1337,54 @@ def test_two_param_array_matches_scalar(name, draw, pa, pb):
     "name,draw,pa,pb", _LOC_SCALE_DISTS, ids=[d[0] for d in _LOC_SCALE_DISTS]
 )
 def test_two_param_array_applies_per_element(name, draw, pa, pb):
-    # A scale sweep must widen the spread across the result.
-    n = 60000
-    lo = np.full(n, pa)
-    hi = np.linspace(pb, pb * 4.0, n)
-    out = draw(rnd.MKLRandomState(99), lo, hi, None)
-    first, last = out[: n // 4], out[-n // 4 :]
-    assert np.std(last) > np.std(first), (
-        f"{name}: per-element parameters do not appear to be applied"
+    loc = np.linspace(pa, pa + 2.0, 3)[:, None]
+    scale = np.linspace(pb, pb * 4.0, 4)
+    shape = (3, 4)
+    reference = rnd.MKLRandomState(99)
+    if name == "lognormal":
+        standard = reference.standard_normal(shape)
+        expected = np.exp(loc + scale * standard)
+    else:
+        standard = draw(reference, 0.0, 1.0, shape)
+        width = scale - loc if name == "uniform" else scale
+        expected = loc + width * standard
+    out = draw(rnd.MKLRandomState(99), loc, scale, None)
+    assert out.shape == shape
+    np.testing.assert_allclose(
+        out,
+        expected,
+        rtol=1e-12,
+        atol=1e-12,
+        err_msg=f"{name}: per-element parameters are not applied correctly",
+    )
+
+
+@pytest.mark.parametrize(
+    "name,method,normal_method",
+    [
+        ("normal", "ICDF", "ICDF"),
+        ("normal", "BoxMuller", "BoxMuller"),
+        ("normal", "BoxMuller2", "BoxMuller2"),
+        ("lognormal", "ICDF", "ICDF"),
+        ("lognormal", "BoxMuller", "BoxMuller2"),
+    ],
+)
+@pytest.mark.parametrize("size", [None, (2, 3), (3, 3)])
+def test_normal_family_array_methods(name, method, normal_method, size):
+    loc = np.array([-0.5, 0.0, 0.5])
+    scale = np.array([0.5, 1.0, 1.5])
+    shape = loc.shape if size is None else size
+    reference = rnd.MKLRandomState(1234)
+    state = rnd.MKLRandomState(1234)
+    standard = reference.standard_normal(shape, method=normal_method)
+    expected = loc + scale * standard
+    if name == "lognormal":
+        expected = np.exp(expected)
+    actual = getattr(state, name)(loc, scale, size, method=method)
+    assert actual.shape == shape
+    np.testing.assert_allclose(actual, expected, rtol=1e-12, atol=1e-12)
+    np.testing.assert_array_equal(
+        state.random_sample(32), reference.random_sample(32)
     )
 
 
@@ -1381,7 +1421,9 @@ def test_one_param_array_matches_scalar(name, draw, p):
         ((7,), (7,), 7, (7,)),
     ],
 )
-def test_two_param_array_broadcast_shapes(loc_shape, scale_shape, size, expected):
+def test_two_param_array_broadcast_shapes(
+    loc_shape, scale_shape, size, expected
+):
     loc = np.zeros(loc_shape) if loc_shape else 0.0
     scale = np.ones(scale_shape) if scale_shape else 1.0
     assert rnd.MKLRandomState(5).normal(loc, scale, size).shape == expected
@@ -1392,6 +1434,30 @@ def test_two_param_array_size_incompatible():
         rnd.MKLRandomState(5).normal(np.zeros(5), np.ones(5), 3)
 
 
+@pytest.mark.parametrize(
+    "name",
+    [
+        "normal",
+        "uniform",
+        "exponential",
+        "laplace",
+        "gumbel",
+        "logistic",
+        "rayleigh",
+        "lognormal",
+    ],
+)
+@pytest.mark.parametrize("param_shape,size", [((1, 4), (4,)), ((1,), ())])
+def test_array_size_rejects_extra_parameter_dimensions(name, param_shape, size):
+    state = rnd.MKLRandomState(5)
+    reference = rnd.MKLRandomState(5)
+    with pytest.raises(ValueError, match="size is not compatible with inputs"):
+        getattr(state, name)(np.full(param_shape, 0.5), size=size)
+    np.testing.assert_array_equal(
+        state.random_sample(32), reference.random_sample(32)
+    )
+
+
 def test_randomdist_vonmises(randomdist):
     rnd.seed(randomdist.seed, brng=randomdist.brng)
     actual = rnd.vonmises(mu=1.23, kappa=1.54, size=(3, 2))