Cleanup: broadcasting by anntzer · Pull Request #7562 · matplotlib/matplotlib

anntzer · 2016-12-05T00:55:45Z

Rely more on broadcasting.
Use power operator / np.hypot where appropriate.
Use np.pi instead of math.pi.

tacaswell · 2016-12-05T00:59:33Z

examples/animation/unchained.py

 # Generate random data
 data = np.random.uniform(0, 1, (64, 75))
 X = np.linspace(-1, 1, data.shape[-1])
-G = 1.5 * np.exp(-4 * X * X)


In most cases I think this construct is marginally faster.

# float is probably the most common case In [8]: x = np.arange(100000).astype(float) In [9]: %timeit x * x 10000 loops, best of 3: 65.5 µs per loop In [10]: %timeit x ** 2 10000 loops, best of 3: 66.4 µs per loop

(and you can easily get them reversed on another run).

interesting. That is an apparently wrong rule of thumb I have been using for a while.

Why would you think this would be the case? (I am honestly puzzled.)

It dates back to the early days of computers and compilers, when computers were slow and compilers were not so bright. Multiplication is faster than taking a power, in general. But now compilers recognize things like this and find the best algorithm. In the case of numpy, I'm pretty sure there is explicit internal optimization of small integer powers so that we wouldn't have to use that ancient manual optimization.

I respectfully request the children remain off of my lawn. 😈

As @efiring says, numpy hard-codes power(x, 2) to fall back on square(x), along with a handful of other constants (I think (-1, 0, 0.5, 1, 2))

tacaswell · 2016-12-05T01:02:50Z

lib/matplotlib/axes/_axes.py

-
        # make them safe to take len() of
        _left = left
-        left = make_iterable(left)


Does this break unit handling? It is probably better to leave the duck-ish typing here and not force to numpy

I don't know what unit handling is actually supposed to do, but in any case if an object array (or list of objects, or single object) is passed in then atleast_1d will return an object array.

At the top of examples/units/radian_demo.py, I added:

print(x[0]) print(np.atleast_1d(x)[0]) print(np.atleast_1d(x[0]))

and it printed:

[ 0.] in radians [ 0.] in radians [0.0]

so I guess it might have broken something, unit-wise?

This seems due to a useless and incorrect implementation of __array__. Tried to fix the example...

The problem is when the unit is carried on the container class, not the values. This will strip the units and turn it into a plain numpy array. These sorts of things tend to be implemented as numpy array sub-classes (ex pint and yt).

This will also force a copy of pandas Series we get in.

These need to stay as they were or the process_unic_info call needs to be moved above these casts.

atleast_1d calls asanyarray so it passes subclasses through with no problem. It is true that pandas Series do not subclass numpy arrays and get copied, but given that we're going to generate one Rectangle object per entry in the Series I'd say the additional cost of the copy is rather small.

Also, note that the current implementation is actually buggy: bar([1, 2], [3, 4], width=[.8]) works but bar([1, 2], [3, 4], width=np.array([.8])) currently raises an exception due to the fact that width *= nbars works elementwise here. (Can you open a separate issue if you don't think this PR can get merged any time soon, so that it doesn't get lost?)

Edit: Actually asarray doesn't even copy the underlying buffer for pandas series:

In [1]: s = pd.Series([1, 2, 3]); t = np.asarray(s); t[0] = 42; s Out[1]: 0 42 1 2 2 3 dtype: int64

so the point is moot.

QuLogic · 2016-12-05T04:43:18Z

examples/api/custom_projection_example.py

            half_y = 0.5 * y
-            z = np.sqrt(1.0 - quarter_x*quarter_x - half_y*half_y)
-            longitude = 2 * np.arctan((z*x) / (2.0 * (2.0*z*z - 1.0)))
+            z = np.sqrt(1 - quarter_x * quarter_x - half_y * half_y)


Not using **2 on these ones?

QuLogic · 2016-12-05T04:48:56Z

examples/pylab_examples/scatter_masked.py

+area = np.pi * (10 * np.random.rand(N)) ** 2  # 0 to 10 point radiuses
 c = np.sqrt(area)
-r = np.sqrt(x*x + y*y)
+r = np.sqrt(x * x + y * y)


np.hypot?

I don't think we have to use hypot every time, especially in examples, as the function may be somewhat unknown.

QuLogic · 2016-12-05T04:56:12Z

lib/matplotlib/axes/_axes.py

-
        # make them safe to take len() of
        _left = left
-        left = make_iterable(left)


At the top of examples/units/radian_demo.py, I added:

print(x[0]) print(np.atleast_1d(x)[0]) print(np.atleast_1d(x[0]))

and it printed:

[ 0.] in radians [ 0.] in radians [0.0]

so I guess it might have broken something, unit-wise?

QuLogic · 2016-12-05T04:58:06Z

lib/matplotlib/backends/backend_wx.py

            gfx_ctx.DrawText(s, x, y)
        else:
-            rads = angle / 180.0 * math.pi
+            rads = math.radians(angle)


Not deg2rad like the other PR?

math only has radians and degrees, not deg2rad and rad2deg. I seems that both names are used in the codebase (even before my earlier PR).

Oops, I meant to go back and check whether that was math or np and forgot about it.

QuLogic · 2016-12-05T05:04:51Z

lib/matplotlib/projections/geo.py

            half_y = 0.5 * y
-            z = np.sqrt(1.0 - quarter_x*quarter_x - half_y*half_y)
-            longitude = 2 * np.arctan((z*x) / (2.0 * (2.0*z*z - 1.0)))
+            z = np.sqrt(1 - quarter_x * quarter_x - half_y * half_y)


No **2 again?

QuLogic · 2016-12-05T05:07:08Z

lib/matplotlib/tests/test_axes.py

            for i in range(m):
-                w = (i / float(m) - y) * z
-                a[i] += x * np.exp(-w * w)
+                w = (i / m - y) * z


Could be written as a NumPy expression instead of a loop.

QuLogic · 2016-12-05T05:09:21Z

lib/mpl_toolkits/mplot3d/axes3d.py

-                raise ValueError(("Argument 'zs' must be of same size as 'xs' "
-                    "and 'ys' or of size 1."))
-
+        xs, ys, zs = np.broadcast_arrays(*map(np.ma.ravel, [xs, ys, zs]))


This possibly makes any error about incorrect shapes a bit more obscure.

broadcast_to explicitly prints the nonmatching shape in the error message, but not broadcast_arrays. I'll open an issue on numpy but I don't think this should hold up this PR.

(Note how just below we gain checking on the size of dx/dy/dz, which was not present before.)

efiring · 2016-12-05T06:08:21Z

Looks like lots of nice cleanups here, once again. In future, however, I suggest that you reconsider your policy of always surrounding operators by spaces. This is not required by PEP 8, and although I am a fan of white space, I think that in many cases omitting spaces around operators improves readability. E.g., a * b**2 is better than a * b ** 2 and a*b + c*d is better than a * b + c * d. This is mainly for variables with very short names.

anntzer · 2016-12-05T06:34:39Z

Sounds like a reasonable policy, thanks for the notice.

anntzer · 2016-12-05T07:05:37Z

Looks like there the build failure comes from the newly released sphinx 1.5.

QuLogic · 2016-12-05T07:06:30Z

Yep, #7569.

ianthomas23 · 2016-12-05T07:39:27Z

examples/mplot3d/tricontour3d_demo.py

 # Create the mesh in polar coordinates and compute x, y, z.
 radii = np.linspace(min_radius, 0.95, n_radii)
-angles = np.linspace(0, 2*np.pi, n_angles, endpoint=False)
+angles = np.linspace(0, 2 * np.pi, n_angles, endpoint=False)


I am very strongly opposed to unnecessary whitespace changes of this form, and there are quite a lot of them in this PR.

They are also not related to the title and description of the PR.

anntzer · 2016-12-05T09:04:56Z

I got rid of all the trivial whitespace changes. (Lines with both whitespace changes and nonwhitespace changes kept their changes.)

ianthomas23 · 2016-12-06T08:39:50Z

@anntzer Thankyou, but you do need to be consistent. Changing some is worse than changing all or none.

anntzer · 2016-12-06T09:03:40Z

I think it's all fairly consistent now.

anntzer · 2016-12-06T19:34:08Z

There seems to be some issues left with the unit handling code that I need to look into.

tacaswell · 2016-12-29T23:40:04Z

examples/pylab_examples/annotation_demo.py

 ax = fig.add_subplot(111, projection='polar')
 r = np.arange(0, 1, 0.001)
-theta = 2*2*np.pi*r
+theta = 4 * np.pi * r


The 2*2 here may have been pedagogical (ex 2 * tau )

Reverted; however I chose to leave the use of **2 instead of X * X above (unless you feel strongly about it).

no, @efiring explained that one correctly. If it isn't faster, use the more obvious way to write it.

tacaswell · 2016-12-29T23:49:35Z

lib/matplotlib/axes/_axes.py

-
        # make them safe to take len() of
        _left = left
-        left = make_iterable(left)


The problem is when the unit is carried on the container class, not the values. This will strip the units and turn it into a plain numpy array. These sorts of things tend to be implemented as numpy array sub-classes (ex pint and yt).

This will also force a copy of pandas Series we get in.

tacaswell · 2016-12-29T23:54:23Z

lib/matplotlib/axes/_axes.py

-
        # make them safe to take len() of
        _left = left
-        left = make_iterable(left)


These need to stay as they were or the process_unic_info call needs to be moved above these casts.

tacaswell · 2016-12-30T00:00:36Z

lib/matplotlib/stackplot.py

-        y = np.atleast_2d(*args)
-    elif len(args) > 1:
-        y = np.row_stack(args)
+    y = np.row_stack(args)


These are not equivalent

In [18]: np.atleast_2d([1, 2, 3]) Out[18]: array([[1, 2, 3]]) In [19]: np.row_stack([1, 2, 3]) Out[19]: array([[1], [2], [3]])

No, it's np.atleast_2d(*args) (note the unpack) and only in the case where len(args) == 1.

Say args has shape (after casting to an array) (1, x, y, ...).

np.atleast_2d(*args) == np.atleast_2d(<shape (x, y, ...)>) == <shape (x, y, ...)>

np.row_stack(args) also has shape (x, y, ...).

In the case args is 2D (shape (1, x)), both expressions result in a shape (1, x) as well.

tacaswell · 2016-12-30T00:02:10Z

lib/matplotlib/tests/test_axes.py

+            a += x * np.exp(-((np.arange(m) / m - y) * z) ** 2)
        a = np.zeros((m, n))
        for i in range(n):
            for j in range(5):


Why is this inner loop here?

Because whoever came up with that example decided to add some random numbers five times to each column rather than only once...

🐑 Yeah, I am greatly confused by the local operating in-place function...

stackplot_demo2 (from which this function comes) has the slightly more helpful docstring "Return n random Gaussian mixtures, each of length m." I can copy it there, or just inline the function (also in the demo), let me know if you have a preference.

QuLogic · 2017-06-01T04:48:39Z

@tacaswell uses ⚔️ for conflicts...

anntzer · 2017-07-30T07:37:52Z

Rebased.

QuLogic · 2017-07-31T02:21:43Z

Still LGTM, though maybe squash the import fixup into the relevant commits. Might also want to rebase just to get CircleCI+AppVeyor fixes too..

anntzer · 2017-07-31T02:47:01Z

I squashed-rebased everything as splitting the fixup commit across the rebase seems a bit tricky, plus everything is still more or less related.

QuLogic · 2017-07-31T04:45:42Z

I forgot; why did the test image change?

anntzer · 2017-07-31T05:16:02Z

Because the previous behavior was incorrect: see the leftmost green "triangle", which is did not go all the way to the left but now does.

QuLogic · 2017-07-31T05:31:10Z

Ah, I see it now.

QuLogic · 2017-08-23T06:32:47Z

Needs a rebase; I'm also going to dismiss @tacaswell's review which seems to be outdated.

Outdated.

anntzer · 2017-08-23T09:09:41Z

done

dopplershift · 2017-08-23T23:52:29Z

And of course in that time a PR went in that causes conflicts...

anntzer · 2017-08-24T00:24:06Z

and fixed again...

dopplershift · 2017-08-24T19:21:14Z

examples/lines_bars_and_markers/nan_test.py


 t = np.arange(0.0, 1.0 + 0.01, 0.01)
-s = np.cos(2 * 2 * np.pi * t)
+s = np.cos(2 * 2*np.pi * t)


Just curious: why is this the one where you don't have spaces around it?

nm...I guess I see it now.

dopplershift

Just one question about imports...

dopplershift · 2017-08-24T19:37:44Z

lib/matplotlib/projections/polar.py

+import six
+
+from collections import OrderedDict
+import warnings


Why is this import necessary?

Same thing actually for six.

Probably some rebase made the thing irrelevant. Fixed.

Also fixes a bug in fill_between with masked data. In the modified test figures, the area in green is supposed to correspond to the part of the hatched area where the curve is below y=2. The new behavior is the correct one. Also fixes cbook._reshape2D for scalar object inputs. Before the fix, `plt.hist(None)` would fail with `x must have 2 or fewer dimensions`, which it does have. Now it fails with a bit later with `unsupported operands type(s) for +: 'NoneType' and 'float'`, which is hopefully clearer.

dopplershift

Just waiting on CI to pass...

tacaswell reviewed Dec 5, 2016

View reviewed changes

anntzer force-pushed the cleanup-broadcasting branch 6 times, most recently from 7b3171b to f15e8f4 Compare December 5, 2016 04:31

QuLogic reviewed Dec 5, 2016

View reviewed changes

anntzer force-pushed the cleanup-broadcasting branch from f15e8f4 to 4a15fec Compare December 5, 2016 05:23

ianthomas23 reviewed Dec 5, 2016

View reviewed changes

anntzer force-pushed the cleanup-broadcasting branch from 3fd7fea to d01ce65 Compare December 5, 2016 09:03

anntzer force-pushed the cleanup-broadcasting branch 3 times, most recently from 6803631 to f0c3b4f Compare December 5, 2016 19:27

anntzer force-pushed the cleanup-broadcasting branch from f0c3b4f to 9d6d81b Compare December 6, 2016 09:03

anntzer force-pushed the cleanup-broadcasting branch from 9d6d81b to b69c4f6 Compare December 6, 2016 09:29

anntzer force-pushed the cleanup-broadcasting branch from b69c4f6 to dfa77b9 Compare December 7, 2016 03:39

tacaswell added this to the 2.1 (next point release) milestone Dec 29, 2016

tacaswell reviewed Dec 29, 2016

View reviewed changes

tacaswell previously requested changes Dec 30, 2016

View reviewed changes

anntzer force-pushed the cleanup-broadcasting branch from dfa77b9 to 694ea11 Compare December 30, 2016 00:13

anntzer force-pushed the cleanup-broadcasting branch from 4ae08f2 to 79bc88d Compare June 1, 2017 04:51

anntzer force-pushed the cleanup-broadcasting branch from 79bc88d to 9a077e8 Compare July 29, 2017 23:41

anntzer force-pushed the cleanup-broadcasting branch from 86a86e2 to 33fef83 Compare July 31, 2017 02:45

QuLogic approved these changes Jul 31, 2017

View reviewed changes

anntzer force-pushed the cleanup-broadcasting branch from 33fef83 to aee5e1e Compare August 23, 2017 09:09

anntzer force-pushed the cleanup-broadcasting branch from aee5e1e to 1e68491 Compare August 24, 2017 00:23

dopplershift reviewed Aug 24, 2017

View reviewed changes

dopplershift requested changes Aug 24, 2017

View reviewed changes

anntzer force-pushed the cleanup-broadcasting branch from 1e68491 to 5348ce9 Compare August 24, 2017 20:52

dopplershift approved these changes Aug 24, 2017

View reviewed changes

QuLogic merged commit 7cf904d into matplotlib:master Aug 27, 2017

anntzer deleted the cleanup-broadcasting branch August 27, 2017 22:55

This was referenced Jan 26, 2018

Use np.hypot wherever possible. #10322

Merged

Various examples updates. #10326

Merged

anntzer mentioned this pull request Jan 12, 2019

FIX: (broken)bar(h) math before units #12903

Merged

6 tasks

jklymak mentioned this pull request Mar 3, 2019

fill_between interpolation & nan issue #11781

Closed

thangleiter mentioned this pull request Jul 7, 2020

mplot3d: add_collection3d issues #17755

Closed

Uh oh!

Conversation

anntzer commented Dec 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer Dec 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiring commented Dec 5, 2016

Uh oh!

anntzer commented Dec 5, 2016

Uh oh!

anntzer commented Dec 5, 2016

Uh oh!

QuLogic commented Dec 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer commented Dec 5, 2016

Uh oh!

ianthomas23 commented Dec 6, 2016

anntzer commented Dec 5, 2016 •

edited

Loading

eric-wieser Dec 15, 2017 •

edited

Loading

anntzer Dec 5, 2016 •

edited

Loading