Skip to content

MAINT: Large overhead in some random functions #15511

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

Jump to bottom
Merged
merged 3 commits into from
Feb 6, 2020
Merged

MAINT: Large overhead in some random functions #15511

merged 3 commits into from
Feb 6, 2020

Conversation

przemb
Copy link
Contributor

@przemb przemb commented Feb 4, 2020 •
edited by charris
Loading

Fixes #15460

  • slow calls to np.dtype.name replaced with np.dtype,
  • mtrand.pyx and _generator.pyx updated
  • test _test_warns_byteorder_ updated

@charris charris added this to the 1.18.2 release milestone Feb 4, 2020
@charris charris changed the title BUG: Large overhead in some random functions #15460 MAINT: Large overhead in some random functions #15460 Feb 4, 2020
@charris
Copy link
Member

charris commented Feb 4, 2020

Do these fixes change the byte stream?

@przemb przemb changed the title MAINT: Large overhead in some random functions #15460 MAINT: Large overhead in some random functions Feb 4, 2020
@seberg
Copy link
Member

seberg commented Feb 4, 2020

I am not sure this is a regression, since I think the legacy version did not have the dtype kwarg normally? But we might as well backport probably.

This should not change the byte streams, it should be purely maintenance in that sense. It might be nice to see if at least some of these are picked up by the benchmarks (if there are benchmarks drawing even a medium number from some of these distributions, I would expect so, however).

@przemb
MAINT: Large overhead in some random functions numpy#15460
a15dc30 
slow calls to np.dtype.name replaced with np.dtype,
mtrand.pyx and _generator.pyx updated,
test test_warns_byteorder updated

before:
%timeit rs.random(): 520 ns ± 33.1 ns per loop
%timeit rg.random(): 6.36 µs ± 222 ns per loop

after:
%timeit rs.random(): 453 ns ± 6.95 ns per loop
%timeit rg.random(): 594 ns ± 9.66 ns per loop
@przemb przemb force-pushed the random_speedup_15460 branch from d307b59 to a15dc30 Compare February 5, 2020 00:20
@przemb przemb requested a review from eric-wieser February 5, 2020 07:34
@przemb przemb mentioned this pull request Feb 5, 2020
Closed
mattip
mattip reviewed Feb 5, 2020
View reviewed changes
@mattip
Copy link
Member

mattip commented Feb 5, 2020

I tried to set up my laptop for consistent benchmark running (configuring the system so I could do sudo cpupower frequency-set --governor userspace then setting sudo cpupower frequency-set -f 2.1GHz which is much less than the top frequency), then running

python runtests.py --bench-compare HEAD

resulted in no significant changes.

@eric-wieser
Copy link
Member

eric-wieser commented Feb 5, 2020 •
edited
Loading

Likely means we don't have useful benchmarks. Here's a micro benchmark whoing a big improvement:

# factor of 2 savings to be gained with pre-constructed dtypes.
In [10]: %timeit dt == dt2
84.2 ns ± 4.24 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [11]: %timeit dt == np.float32
191 ns ± 30.5 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [12]: %timeit dt.name == 'float32'
3.87 µs ± 688 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

@mattip
Copy link
Member

mattip commented Feb 5, 2020

Fixes #15460. I can see a big improvement on a microbenchmark:

$ python runtests.py --ipython
...
In [1]: rs = np.random.RandomState() 
   ...: rg = np.random.default_rng()
In [2]: %timeit rs.random()                                                                                                                                                              
191 ns ± 1.11 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [3]: %timeit rg.random()                                                                                                                                                              
2.54 µs ± 36.6 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

After this changeset (a15dc30)

In [2]: %timeit rs.random()                                                                                                                                                              
188 ns ± 2.07 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [3]: %timeit rg.random()                                                                                                                                                              
239 ns ± 0.968 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

@przemb przemb requested review from eric-wieser and mattip February 5, 2020 14:05
eric-wieser
eric-wieser reviewed Feb 5, 2020
View reviewed changes
numpy/random/_generator.pyx
@@ -312,13 +311,13 @@ cdef class Generator:

"""
cdef double temp
key = np.dtype(dtype).name
if key == 'float64':
_dtype = np.dtype(dtype)
Copy link
Member

@eric-wieser eric-wieser Feb 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Would normally spell this

Suggested change
_dtype = np.dtype(dtype)
dtype = np.dtype(dtype)

Does cython not like that?

(same throughout)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I change it, test are failing... seems that it is not proper.

Copy link
Member

@eric-wieser eric-wieser Feb 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Do you have a link to the CI failure?

Edit: caused diagnosed here: #15511 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

We need the original value so we can raise an error with it later. What about descr or concrete_dtype?

@przemb przemb force-pushed the random_speedup_15460 branch from 484ffce to df27b88 Compare February 5, 2020 16:47
@przemb przemb force-pushed the random_speedup_15460 branch from 69f6edd to 7d181f3 Compare February 5, 2020 18:27
eric-wieser
eric-wieser requested changes Feb 5, 2020
View reviewed changes
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Docs still need updating as mentioned in https://github.com/numpy/numpy/pull/15511/files#r375407384

@przemb przemb force-pushed the random_speedup_15460 branch 2 times, most recently from 256a017 to 7821fcc Compare February 5, 2020 20:17
@przemb przemb requested a review from eric-wieser February 5, 2020 20:20
@przemb przemb force-pushed the random_speedup_15460 branch 2 times, most recently from 5c3c057 to b0fa44a Compare February 6, 2020 10:12
@przemb przemb force-pushed the random_speedup_15460 branch from b0fa44a to bf6ba07 Compare February 6, 2020 10:35
@przemb przemb force-pushed the random_speedup_15460 branch from bf6ba07 to 29fb22c Compare February 6, 2020 10:59
eric-wieser
eric-wieser approved these changes Feb 6, 2020
View reviewed changes
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Thanks for working through the doc fixes!

mattip
mattip reviewed Feb 6, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed Feb 6, 2020
View reviewed changes
@przemb przemb force-pushed the random_speedup_15460 branch from 29fb22c to deb8571 Compare February 6, 2020 14:06
eric-wieser
eric-wieser approved these changes Feb 6, 2020
View reviewed changes
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

LGTM

@mattip mattip merged commit dae4f67 into numpy:master Feb 6, 2020
@mattip
Copy link
Member

mattip commented Feb 6, 2020

Thanks @przemb

This was referenced Mar 1, 2020
Closed
Merged
@charris charris added component: numpy.random and removed 09 - Backport-Candidate PRs tagged should be backported labels Mar 1, 2020
@charris charris removed this from the 1.18.2 release milestone Mar 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rkern rkern rkern left review comments

@mattip mattip mattip left review comments

@eric-wieser eric-wieser eric-wieser approved these changes

Assignees
No one assigned
Labels
03 - Maintenance 06 - Regression component: numpy.random
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG: Large overhead in some random functions
6 participants
@przemb @charris @seberg @mattip @eric-wieser @rkern