NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.3k

Improve speed in some modules#22678

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Draft

oscargus wants to merge1 commit intomatplotlib:main

base:main

Choose a base branch

fromoscargus:speedimprovements

Draft

Improve speed in some modules#22678

oscargus wants to merge1 commit intomatplotlib:mainfromoscargus:speedimprovements

Conversation

Copy link

Member

oscargus commentedMar 21, 2022•
edited
Loading

PR Summary

Enable pre-computation during compilation and remove redundant computations.

Before:

In [7]: %timeit Path.circle()25 µs ± 78.9 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)

After:

In [28]: %timeit Path.circle()21.6 µs ± 72.1 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)

PR Checklist

Tests and Styling

Has pytest style unit tests (andpytest passes).
IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

New features are documented, with examples if plot related.
New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).

oscargus added the Performance label

Mar 21, 2022

oscargus force-pushed thespeedimprovements branch from0f9decf to295bef0Compare

March 21, 2022 10:07

Improve speed

7b8fd5e

oscargus force-pushed thespeedimprovements branch from295bef0 to7b8fd5eCompare

March 21, 2022 10:41

QuLogic reviewed

Mar 21, 2022

View reviewed changes

lib/matplotlib/projections/polar.pyShow resolvedHide resolved

tacaswell added this to thev3.6.0 milestone

Mar 22, 2022

Copy link

Member

tacaswell commentedMar 22, 2022

I'm actually quite surprised (but remember being surprised by this before) that raising to a fraction power is pretty fast:

$ipythonPython3.11.0a6+ (heads/main:29624e769c,Mar182022,18:36:12) [GCC11.2.0]Type'copyright','credits'or'license'formoreinformationIPython8.2.0.dev--AnenhancedInteractivePython.Type'?'forhelp.In [1]:importnumpyasnpIn [2]:%timeitnp.sqrt(52)476ns ±1.14nsperloop (mean ±std.dev.of7runs,1,000,000loopseach)In [3]:%timeit52** (1/2)3.12ns ±0.018nsperloop (mean ±std.dev.of7runs,100,000,000loopseach)In [4]:importmathIn [5]:%timeitmath.sqrt(52)25.4ns ±0.125nsperloop (mean ±std.dev.of7runs,10,000,000loopseach)In [6]:s=math.sqrtIn [7]:%timeits(52)25.4ns ±0.0733nsperloop (mean ±std.dev.of7runs,10,000,000loopseach)In [8]:%timeits(52.0)14.9ns ±0.0463nsperloop (mean ±std.dev.of7runs,100,000,000loopseach)In [9]:%timeit52.0** (1/2)3.15ns ±0.00981nsperloop (mean ±std.dev.of7runs,100,000,000loopseach)In [10]:

Copy link

Member

timhoffm commentedMar 22, 2022

Powers of constants are pre-computed.
Powers of variables use BINARY_POWER
I anticipate that the function call overhead formath.sqrt() is relevant on the ns timescale.

In [10]: dis.dis('52 ** 0.5')  1           0 LOAD_CONST               0 (7.211102550927978)              2 RETURN_VALUEIn [11]: dis.dis('a ** 0.5')  1           0 LOAD_NAME                0 (a)              2 LOAD_CONST               0 (0.5)              4 BINARY_POWER              6 RETURN_VALUEIn [12]: dis.dis('math.sqrt(52)')  1           0 LOAD_NAME                0 (math)              2 LOAD_METHOD              1 (sqrt)              4 LOAD_CONST               0 (52)              6 CALL_METHOD              1              8 RETURN_VALUEIn [13]: dis.dis('math.sqrt(a)')  1           0 LOAD_NAME                0 (math)              2 LOAD_METHOD              1 (sqrt)              4 LOAD_NAME                2 (a)              6 CALL_METHOD              1              8 RETURN_VALUE

Copy link

Member

Choose a reason for hiding this comment

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

I don't think this matters wrt speed (does it?); also I prefer a list here as this matches "more" the return type of np.percentile below (a ndarray is closer to a list than to a tuple.)

anntzer reviewed

Apr 24, 2022

View reviewed changes

lib/matplotlib/cbook/__init__.py

		@@ -1221,7 +1222,8 @@ def _compute_conf_interval(data, med, iqr, bootstrap):
		stats['mean'] = np.mean(x)

		# medians and quartiles
		q1, med, q3 = np.percentile(x, [25, 50, 75])
		percentiles = (25, 50, 75)
		q1, med, q3 = np.percentile(x, percentiles)

Copy link

Contributor

Choose a reason for hiding this comment

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

ditto, leave as before.

anntzer reviewed

Apr 24, 2022

View reviewed changes

lib/matplotlib/cbook/__init__.py

		@@ -1169,8 +1169,9 @@ def _compute_conf_interval(data, med, iqr, bootstrap):
		else:

		N = len(data)
		notch_min = med - 1.57 * iqr / np.sqrt(N)
		notch_max = med + 1.57 * iqr / np.sqrt(N)
		half_height = 1.57 * iqr / (N ** (1 / 2))

Copy link

Contributor

Choose a reason for hiding this comment

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

I guess you could inline len(data) into the computation here.

anntzer reviewed

Apr 24, 2022

View reviewed changes

lib/matplotlib/path.py


		vertices = np.array([[0.0, -1.0],

Copy link

Contributor

anntzerApr 24, 2022•
edited
Loading

Choose a reason for hiding this comment

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

I guess I would just write this is asvertices = np.array([...]) without even bothering with thedtype=float which is unneeded; this also avoids a temporary variable (mostly avoids having to keep track of it mentally). Ditto below.

timhoffm reviewed

Apr 25, 2022

View reviewed changes

lib/matplotlib/markers.py

		@@ -677,7 +677,7 @@ def _set_pentagon(self):
		self._path = polypath
		else:
		verts = polypath.vertices
		y = (1 +np.sqrt(5)) / 4.
		y = (1 +5 ** (1 / 2)) / 4.

Copy link

Member

timhoffmApr 25, 2022

Choose a reason for hiding this comment

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

I'm not convinced of all these sqrt micro-optimizations. IMHO it makes the code less readable, and the performance gain is minimal. For example here

The sqrt is less than 2% of the the time of the immediate surrounding code in the function:

In [21]: %%timeit    ...: Affine2D().scale(0.5)    ...: polypath = Path.unit_regular_polygon(5)    ...: verts = polypath.vertices    ...: top = Path(verts[[0, 1, 4, 0]])    ...: bottom = Path(verts[[1, 2, 3, 4, 1]])    ...: left = Path([verts[0], verts[1], verts[2], [0, -y], verts[0]])    ...: right = Path([verts[0], verts[4], verts[3], [0, -y], verts[0]])    ...: 61.1 µs ± 228 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)In [22]: %timeit np.sqrt(5)2.36 µs ± 35 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

and that function_set_pentagon() again is only used to create a new marker object, which in itself is quite rare. So I bet the performance gain is not measurable in any real example.

QuLogic modified the milestones:v3.6.0,v3.7.0

Aug 24, 2022

Copy link

Member

timhoffm commentedDec 15, 2022•
edited
Loading

@oscargus Thanks for trying to speed the code up, However, as said before:

I'm not convinced of all these sqrt micro-optimizations. IMHO it makes the code less readable, and the performance gain is minimal.

Unless we can show there is a measurable performance gain in real situations, I propose to close this.

tacaswell modified the milestones:v3.7.0,v3.8.0

Jan 4, 2023

github-actionsbot added the status: needs rebase label

Jan 10, 2023

jklymak marked this pull request as draft

January 30, 2023 23:01

Copy link

Member

jklymak commentedJan 30, 2023

@oscargus is it OK to close this? I agree that some of the optimizations are marginal at best... In either case, moving to draft ;-)

QuLogic mentioned this pull request

Apr 29, 2023

ci: Increase retry count on PR conflict check#25788

Merged

ksunden modified the milestones:v3.8.0,future releases

Aug 8, 2023

Labels

Performance status: needs rebase

7 participants

		@@ -1150,7 +1150,7 @@ def boxplot_stats(X, whis=1.5, bootstrap=None, labels=None,
		def _bootstrap_median(data, N=5000):
		# determine 95% confidence intervals of the median
		M = len(data)
		percentiles =[2.5, 97.5]
		percentiles =(2.5, 97.5)

Movatterモバイル変換

Uh oh!

Improve speed in some modules#22678

Are you sure you want to change the base?

Improve speed in some modules#22678

Uh oh!

Conversation

oscargus commentedMar 21, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Uh oh!

tacaswell commentedMar 22, 2022

Uh oh!

timhoffm commentedMar 22, 2022

Uh oh!

QuLogic commentedMar 22, 2022

Uh oh!

anntzerApr 24, 2022

Choose a reason for hiding this comment

Uh oh!

anntzerApr 24, 2022

Choose a reason for hiding this comment

Uh oh!

anntzerApr 24, 2022

Choose a reason for hiding this comment

Uh oh!

anntzerApr 24, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timhoffmApr 25, 2022

Choose a reason for hiding this comment

Uh oh!

timhoffm commentedDec 15, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

jklymak commentedJan 30, 2023

Uh oh!

Uh oh!

oscargus commentedMar 21, 2022•
edited
Loading

anntzerApr 24, 2022•
edited
Loading

timhoffm commentedDec 15, 2022•
edited
Loading