8000 ENH: Make 'low' optional in randint by gfyoung · Pull Request #7151 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

ENH: Make 'low' optional in randint #7151

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

ENH: Make 'low' optional in randint #7151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions benchmarks/benchmarks/bench_random.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,11 @@ class Randint(Benchmark):

def time_randint_fast(self):
"""Compare to uint32 below"""
np.random.randint(0, 2**30, size=10**5)
np.random.randint(low=0, high=2**30, size=10**5)
Copy link
Contributor
@madphysicist madphysicist Feb 4, 2016

Choose a reason for hiding this comment

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

I think you should keep the invocation as it was before to ensure backwards compatibility. The same goes for all the other benchmarks and tests you modified in a similar way, especially the ones that just use the function but don't explicitly test it, such as test_multiarray.py, test_mem_overlap.py, etc. If necessary, add a couple more to use the new functionality instead of changing the old ones.

Copy link
Contributor Author
@gfyoung gfyoung Feb 4, 2016

Choose a reason for hiding this comment

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

I'm not sure I follow what backwards compatibility you are trying to enforce here. I see no reason why tests should continue to use a "dated" API.

Copy link
Contributor
@madphysicist madphysicist Feb 4, 2016

Choose a reason for hiding this comment

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

The API is not dated since I am going to guess that most existing code calls this function as np.random.randint(somenumber) and np.random.randint(somenumber, someothernumber). Just because the new version is a useful convenience does not mean that the old one is invalid or even less desirable. Removing the old calling convention from so many of the tests is just asking for trouble later down the line.

Copy link
Contributor Author
@gfyoung gfyoung Feb 4, 2016

Choose a reason for hiding this comment

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

The API would become dated if this PR is merged because low no longer is a positional argument. While it may be worth adding a test to ensure the non-keyword argument call still works, I don't think it should persist in the code-base with this change.

Copy link
Contributor
@juliantaylor juliantaylor Feb 4, 2016

Choose a reason for hiding this comment

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

changing non-test code is imo ok, but when changing test code you need to make sure that no coverage of the old still supported way of calling it is lost. It is not obvious if that has been done.
Please add new test cases for the old way of calling the function if you have removed them.

I may have overlooked it but I also see no tests for the new None cases

Copy link
Contributor Author
@gfyoung gfyoung Feb 4, 2016

Choose a reason for hiding this comment

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

@juliantaylor : The tests are here

Copy link
Member
@ahaldane ahaldane Feb 17, 2016

Choose a reason for hiding this comment

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

I'm also not very enthusuastic about changing all the old unit tests to use the new keywords.

Copy link
Contributor Author
@gfyoung gfyoung Feb 17, 2016

Choose a reason for hiding this comment

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

IMHO unit tests should always use the most up-to-date versions of the API for clarity and consistency. That is why I put one test in this PR for backwards compatibility in which I call the function using the "old" way.

Copy link
Member
@ahaldane ahaldane Feb 17, 2016

Choose a reason for hiding this comment

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

I think guaranteeing that old functionality hasn't broken is more important than checking that new functionality works. I favor virtually never changing old unit tests, unless there is an intentional backwards incompatible change.

I would be OK with you leaving the old unit tests alone, but duplicating some of them and adding keywords in the duplicates, if your new unit tests below don't cover those cases.

Copy link
Contributor Author
@gfyoung gfyoung Feb 17, 2016

Choose a reason for hiding this comment

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

I specifically wrote a test here to address that concern. Maybe it should be expanded, but I don't follow why you would want to have all of your other tests testing the old functionality.


def time_randint_slow(self):
"""Compare to uint32 below"""
np.random.randint(0, 2**30 + 1, size=10**5)
np.random.randint(low=0, high=2**30 + 1, size=10**5)


class Randint_dtype(Benchmark):
Expand All @@ -59,9 +59,9 @@ def setup(self, name):

def time_randint_fast(self, name):
high = self.high[name]
np.random.randint(0, high, size=10**5, dtype=name)
np.random.randint(low=0, high=high, size=10**5, dtype=name)

def time_randint_slow(self, name):
high = self.high[name]
np.random.randint(0, high + 1, size=10**5, dtype=name)
np.random.randint(low=0, high=high + 1, size=10**5, dtype=name)

12 changes: 12 additions & 0 deletions doc/release/1.12.0-notes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,5 +104,17 @@ Added 'doane' and 'sqrt' estimators to ``histogram`` via the ``bins`` argument.
Changes
=======

``np.randint`` can handle more ``low=None`` and ``high=None`` combinations
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
If ``low=None`` and ``high=None``, random numbers will be generated over the
range [``lowbnd``, ``highbnd``), where ``lowbnd`` and ``highbnd`` equal
``np.iinfo(dtype).min`` and ``np.iinfo(dtype).max`` respectively.

If only ``low=None``, random numbers will be generated over the range [``lowbnd``,
``high``), where ``lowbnd`` is defined as previously. If only ``high=None``, random
numbers will still be generated over the range [``0``, ``low``) provided ``low`` >= 0.
Otherwise, numbers will be generated over [``low``, ``highbnd``), where ``highbnd`` is
defined as previously.

Deprecations
============
2 changes: 1 addition & 1 deletion numpy/add_newdocs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3680,7 +3680,7 @@ def luf(lamdaexpr, *args, **kwargs):

Examples
--------
>>> x = np.random.randint(9, size=(3, 3))
>>> x = np.random.randint(low=9, size=(3, 3))
>>> x
Copy link
Contributor Author
@gfyoung gfyoung Feb 4, 2016

Choose a reason for hiding this comment

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

I'm not sure your comment quite applies here because this isn't even documentation for np.randint where you are commenting.

Copy link
Contributor
@madphysicist madphysicist Feb 4, 2016

Choose a reason for hiding this comment

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

Agreed. Removed.

array([[3, 1, 7],
[2, 8, 3],
Expand Down
40 changes: 20 additions & 20 deletions numpy/core/tests/test_mem_overlap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,19 +110,19 @@ def test_diophantine_fuzz():
numbers = []
while min(feasible_count, infeasible_count) < min_count:
# Ensure big and small integer problems
A_max = 1 + rng.randint(0, 11, dtype=np.intp)**6
U_max = rng.randint(0, 11, dtype=np.intp)**6
A_max = 1 + rng.randint(low=0, high=11, dtype=np.intp)**6
U_max = rng.randint(low=0, high=11, dtype=np.intp)**6

A_max = min(max_int, A_max)
U_max = min(max_int-1, U_max)

A = tuple(rng.randint(1, A_max+1, dtype=np.intp)
A = tuple(rng.randint(low=1, high=A_max+1, dtype=np.intp)
for j in range(ndim))
U = tuple(rng.randint(0, U_max+2, dtype=np.intp)
U = tuple(rng.randint(low=0, high=U_max+2, dtype=np.intp)
for j in range(ndim))

b_ub = min(max_int-2, sum(a*ub for a, ub in zip(A, U)))
b = rng.randint(-1, b_ub+2, dtype=np.intp)
b = rng.randint(low=-1, high=b_ub+2, dtype=np.intp)

if ndim == 0 and feasible_count < min_count:
b = 0
Expand Down Expand Up @@ -260,9 +260,9 @@ def check_may_share_memory_easy_fuzz(get_max_work, same_steps, min_count):
rng = np.random.RandomState(1234)

def random_slice(n, step):
start = rng.randint(0, n+1, dtype=np.intp)
stop = rng.randint(start, n+1, dtype=np.intp)
if rng.randint(0, 2, dtype=np.intp) == 0:
start = rng.randint(low=0, high=n+1, dtype=np.intp)
stop = rng.randint(low=start, high=n+1, dtype=np.intp)
if rng.randint(low=0, high=2, dtype=np.intp) == 0:
stop, start = start, stop
step *= -1
return slice(start, stop, step)
Expand All @@ -271,14 +271,14 @@ def random_slice(n, step):
infeasible = 0

while min(feasible, infeasible) < min_count:
steps = tuple(rng.randint(1, 11, dtype=np.intp)
if rng.randint(0, 5, dtype=np.intp) == 0 else 1
steps = tuple(rng.randint(low=1, high=11, dtype=np.intp)
if rng.randint(low=0, high=5, dtype=np.intp) == 0 else 1
for j in range(x.ndim))
if same_steps:
steps2 = steps
else:
steps2 = tuple(rng.randint(1, 11, dtype=np.intp)
if rng.randint(0, 5, dtype=np.intp) == 0 else 1
steps2 = tuple(rng.randint(low=1, high=11, dtype=np.intp)
if rng.randint(low=0, high=5, dtype=np.intp) == 0 else 1
for j in range(x.ndim))

t1 = np.arange(x.ndim)
Expand Down Expand Up @@ -378,9 +378,9 @@ def test_internal_overlap_slices():
rng = np.random.RandomState(1234)

def random_slice(n, step):
start = rng.randint(0, n+1, dtype=np.intp)
stop = rng.randint(start, n+1, dtype=np.intp)
if rng.randint(0, 2, dtype=np.intp) == 0:
start = rng.randint(low=0, high=n+1, dtype=np.intp)
stop = rng.randint(low=start, high=n+1, dtype=np.intp)
if rng.randint(low=0, high=2, dtype=np.intp) == 0:
stop, start = start, stop
step *= -1
return slice(start, stop, step)
Expand All @@ -389,8 +389,8 @@ def random_slice(n, step):
min_count = 5000

while cases < min_count:
steps = tuple(rng.randint(1, 11, dtype=np.intp)
if rng.randint(0, 5, dtype=np.intp) == 0 else 1
steps = tuple(rng.randint(low=1, high=11, dtype=np.intp)
if rng.randint(low=0, high=5, dtype=np.intp) == 0 else 1
for j in range(x.ndim))
t1 = np.arange(x.ndim)
rng.shuffle(t1)
Expand Down Expand Up @@ -474,11 +474,11 @@ def test_internal_overlap_fuzz():
rng = np.random.RandomState(1234)

while min(overlap, no_overlap) < min_count:
ndim = rng.randint(1, 4, dtype=np.intp)
ndim = rng.randint(low=1, high=4, dtype=np.intp)

strides = tuple(rng.randint(-1000, 1000, dtype=np.intp)
strides = tuple(rng.randint(low=-1000, high=1000, dtype=np.intp)
for j in range(ndim))
shape = tuple(rng.randint(1, 30, dtype=np.intp)
shape = tuple(rng.randint(low=1, high=30, dtype=np.intp)
for j in range(ndim))

a = as_strided(x, strides=strides, shape=shape)
Expand Down
4 changes: 2 additions & 2 deletions numpy/core/tests/test_multiarray.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2017,8 +2017,8 @@ def test_partition_fuzz(self):
for i in range(1, j - 2):
d = np.arange(j)
np.random.shuffle(d)
d = d % np.random.randint(2, 30)
idx = np.random.randint(d.size)
d = d % np.random.randint(low=2, high=30)
idx = np.random.randint(low=d.size)
kth = [0, idx, i, i + 1]
tgt = np.sort(d)[kth]
assert_array_equal(np.partition(d, kth)[kth], tgt,
Expand Down
4 changes: 2 additions & 2 deletions numpy/core/tests/test_numeric.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1004,8 +1004,8 @@ class TestIndex(TestCase):
def test_boolean(self):
a = rand(3, 5, 8)
V = rand(5, 8)
g1 = randint(0, 5, size=15)
g2 = randint(0, 8, size=15)
g1 = randint(low=0, high=5, size=15)
g2 = randint(low=0, high=8, size=15)
V[g1, g2] = -V[g1, g2]
assert_((np.array([a[0][V > 0], a[1][V > 0], a[2][V > 0]]) == a[:, V > 0]).all())

Expand Down
12 changes: 6 additions & 6 deletions numpy/lib/tests/test_arrayterator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@ def test():
np.random.seed(np.arange(10))

# Create a random array
ndims = randint(5)+1
shape = tuple(randint(10)+1 for dim in range(ndims))
ndims = randint(low=5)+1
shape = tuple(randint(low=10)+1 for dim in range(ndims))
els = reduce(mul, shape)
a = np.arange(els)
a.shape = shape

buf_size = randint(2*els)
buf_size = randint(low=2*els)
b = Arrayterator(a, buf_size)

# Check that each block has at most ``buf_size`` elements
Expand All @@ -30,9 +30,9 @@ def test():
assert_(list(b.flat) == list(a.flat))

# Slice arrayterator
start = [randint(dim) for dim in shape]
stop = [randint(dim)+1 for dim in shape]
step = [randint(dim)+1 for dim in shape]
start = [randint(low=dim) for dim in shape]
stop = [randint(low=dim)+1 for dim in shape]
step = [randint(low=dim)+1 for dim in shape]
slice_ = tuple(slice(*t) for t in zip(start, stop, step))
c = b[slice_]
d = a[slice_]
Expand Down
2 changes: 1 addition & 1 deletion numpy/lib/tests/test_function_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2571,7 +2571,7 @@ def test_axis_keyword(self):
[0, 1],
[6, 7],
[4, 5]])
for a in [a3, np.random.randint(0, 100, size=(2, 3, 4))]:
for a in [a3, np.random.randint(low=0, high=100, size=(2, 3, 4))]:
orig = a.copy()
np.median(a, axis=None)
for ax in range(a.ndim):
Expand Down
2 changes: 1 addition & 1 deletion numpy/lib/tests/test_nanfunctions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,7 @@ def test_small_large(self):
for s in [5, 20, 51, 200, 1000]:
d = np.random.randn(4, s)
# Randomly set some elements to NaN:
w = np.random.randint(0, d.size, size=d.size // 5)
w = np.random.randint(low=0, high=d.size, size=d.size // 5)
d.ravel()[w] = np.nan
d[:,0] = 1. # ensure at least one good value
# use normal median without nans to compare
Expand Down
4 changes: 2 additions & 2 deletions numpy/lib/tests/test_regression.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,13 +147,13 @@ def test_large_fancy_indexing(self, level=rlevel):
def dp():
n = 3
a = np.ones((n,)*5)
i = np.random.randint(0, n, size=thesize)
i = np.random.randint(low=0, high=n, size=thesize)
a[np.ix_(i, i, i, i, i)] = 0

def dp2():
n = 3
a = np.ones((n,)*5)
i = np.random.randint(0, n, size=thesize)
i = np.random.randint(low=0, high=n, size=thesize)
a[np.ix_(i, i, i, i, i)]

self.assertRaises(ValueError, dp)
Expand Down
2 changes: 1 addition & 1 deletion numpy/lib/tests/test_shape_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ def test_kroncompare(self):
reps = [(2,), (1, 2), (2, 1), (2, 2), (2, 3, 2), (3, 2)]
shape = [(3,), (2, 3), (3, 4, 3), (3, 2, 3), (4, 3, 2, 4), (2, 2)]
for s in shape:
b = randint(0, 10, size=s)
b = randint(low=0, high=10, size=s)
for r in reps:
a = np.ones(r, b.dtype)
large = tile(b, r)
Expand Down
Loading
0