Avoid temporaries when preparing step plots. by anntzer · Pull Request #7454 · matplotlib/matplotlib

anntzer · 2016-11-14T01:58:23Z

The previous implementation of pts_to_{pre,mid,post}step was fairly
unefficient: it allocated a large array during validation (vstack),
then a second array to build the return tuple, then converted the
columns to a tuple, then immediately converted the tuple back to a new
array at the call site (to construct a Path). Instead, just create a
single array and work with it all along.

Also some minor related cleanups (moving imports in lines.py, in
particular suggesting to not expose the individual pts_to_*step
functions anymore (they are not directly used, only via the
STEP_LOOKUP_MAP mapping).

tacaswell · 2016-11-14T03:41:33Z

lib/matplotlib/cbook.py

+    steps = np.zeros((1 + len(args), 2 * len(x) - 1))
+    steps[0, 0::2] = x
+    steps[0, 1::2] = steps[0, 0::2][:-1]
+    for out, y in zip(steps[1:], args):


This is trading memory duplication for a python for-loop. Did you bench mark this it terms of time?

Actually the previous implementation also had a for loop over the arguments during validation (line 2314 args = tuple(np.asanyarray(y) for y in args)). So we're even on that.

But in fact I can even do without that loop -- see latest version.

tacaswell · 2016-11-14T03:43:11Z

lib/matplotlib/cbook.py

-    x = np.asanyarray(x)
-    if x.ndim != 1:
-        raise ValueError("x must be 1 dimensional")
-    if len(args) == 0:


Looks like this no longer raises if there are no y-values. I think I am 👍 on this, but just checking that it was intentional.

Yes, I even removed the test for that (test_step_fails).

tacaswell · 2016-11-14T03:47:35Z

lib/matplotlib/tests/test_cbook.py

-        cbook._step_validation(np.arange(12), 'a')
-
-    with pytest.raises(ValueError):
-        cbook._step_validation(np.arange(12))


This test case got dropped.

See comment above re don't error when no y is passed.

tacaswell · 2016-11-14T03:54:35Z

I am mostly 👍 on this, tad concerned about the immutable -> mutable change in the return type.

tacaswell · 2016-11-14T04:15:21Z

Looks like the tuple return is legacy of how the step logic got factored out the line code. I wrote it and no longer remember why.

+0.5 on removing the pts_to_* function from the lines.py namespace (with an API changes note), but leaving them does little harm.

All of the widows test are failing on conda-build related issues.

QuLogic · 2016-11-14T04:12:37Z

lib/matplotlib/cbook.py

-    return tuple(steps)
+    steps = np.zeros((1 + len(args), 2 * len(x) - 1))
+    steps[0, 0::2] = x
+    steps[0, 1::2] = steps[0, 0::2][:-1]


Is there any reason to use two slices here?

I find this easier to read -- otherwise can you tell immediately whether it should be steps[0, 0:-1:2] or steps[0, 0:-2:2]? Also, I'm not writing x[-1] because that slicing may be not so cheap (I'm looking at you xarray). Same reasons apply below.

Either one works, so I guess that's just up to you...

I'm not sure to which x[-1] you are referring in the second sentence?

QuLogic · 2016-11-14T04:17:49Z

lib/matplotlib/cbook.py

-    return tuple(steps)
+    steps = np.zeros((1 + len(args), 2 * len(x) - 1))
+    steps[0, 0::2] = x
+    steps[0, 1::2] = steps[0, 0::2][1:]


Again, any reason for two slices?

QuLogic · 2016-11-14T04:22:17Z

lib/matplotlib/lines.py

 from __future__ import (absolute_import, division, print_function,
                        unicode_literals)

-import six


This belongs up here.

The PEP8 order is stdlib (warnings), third-party (numpy, six) and internals (everything else). Do we prefer always putting six first? (I don't really care.)

Yes, I know; six is analogous to a future-import in our ordering.

put it back there.

anntzer · 2016-11-14T04:54:06Z

Can we remove them without a deprecation period? :-)
(There's actually quite a few such imports that could be removed; something such as https://pypi.python.org/pypi/metamodule/ could be useful.)

tacaswell · 2016-11-14T05:06:47Z

They have only been there from 1.5.0 on so relatively young. If @anntzer is concerned, I am perfectly happy to leave the imports in place 😉

anntzer · 2016-11-14T06:53:33Z

Too late, they're dead now :-)

tacaswell · 2016-11-14T18:43:07Z

doc/users/whats_new/lines_removed_api.rst

@@ -0,0 +1,6 @@
+Functions removed from the `lines` module


This seems more 'api_changes' than 'whats_new'

tacaswell · 2016-11-14T18:50:07Z

One major issue with this is that it is going to strip units and cast everything to a float.

attn @dopplershift

anntzer · 2016-11-14T18:51:48Z

That (casting everything to a float) was already the case before?

tacaswell · 2016-11-14T19:11:53Z

This is true. Should not hold the PR over that, but we will probably have to address the units sooner or later.

dopplershift · 2016-11-14T19:17:59Z

So long as it's no worse than before, it's fine by me. I'll just keep this in the back of my mind until I need a step plot.

QuLogic · 2016-11-14T22:00:19Z

lib/matplotlib/cbook.py

+    steps[0, 0::2] = x
+    steps[0, 1::2] = steps[0, 0::2][:-1]
+    steps[1:, 0::2] = args
+    steps[1:, 1::2] = steps[1:, ::2][:, 1:]


This is one place where I find two slices more confusing.
Start at 2 and take every other one, is much clearer than take every other one and start at 1 (which is really 2 from the original array).

OK, I squashed the two slices together.

The previous implementation of `pts_to_{pre,mid,post}`step was fairly unefficient: it allocated a large array during validation (`vstack`), then a second array to build the return tuple, then converted the columns to a tuple, then immediately converted the tuple back to a new array at the call site (to construct a Path). Instead, just create a single array and work with it all along. Also some minor related cleanups (moving imports in lines.py, in particular not exposing the individual `pts_to_*step` functions anymore (they are not directly used, only via the `STEP_LOOKUP_MAP` mapping).

anntzer force-pushed the step-converters branch from c6c8293 to b022ab1 Compare November 14, 2016 02:45

tacaswell reviewed Nov 14, 2016

View reviewed changes

tacaswell added this to the 2.1 (next point release) milestone Nov 14, 2016

tacaswell reviewed Nov 14, 2016

View reviewed changes

tacaswell approved these changes Nov 14, 2016

View reviewed changes

tacaswell changed the title ~~Avoid temporaries when preparing step plots.~~ [MRG+1] Avoid temporaries when preparing step plots. Nov 14, 2016

QuLogic reviewed Nov 14, 2016

View reviewed changes

anntzer force-pushed the step-converters branch from b022ab1 to ee3ff2f Compare November 14, 2016 04:35

anntzer force-pushed the step-converters branch 2 times, most recently from 7a0fcc1 to 7f199c7 Compare November 14, 2016 15:37

tacaswell reviewed Nov 14, 2016

View reviewed changes

anntzer force-pushed the step-converters branch from 7f199c7 to 80779ea Compare November 14, 2016 18:56

QuLogic reviewed Nov 14, 2016

View reviewed changes

anntzer force-pushed the step-converters branch from 80779ea to cd2608e Compare November 14, 2016 22:12

QuLogic approved these changes Nov 14, 2016

View reviewed changes

tacaswell merged commit 901562f into matplotlib:master Nov 15, 2016

QuLogic changed the title ~~[MRG+1] Avoid temporaries when preparing step plots.~~ Avoid temporaries when preparing step plots. Nov 15, 2016

anntzer deleted the step-converters branch November 15, 2016 03:56

anntzer mentioned this pull request Oct 20, 2017

[MRG] Replace verbose class with standard logging library #9313

Merged

6 tasks

Uh oh!

Conversation

anntzer commented Nov 14, 2016

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!

tacaswell commented Nov 14, 2016

Uh oh!

tacaswell commented Nov 14, 2016

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 commented Nov 14, 2016

Uh oh!

tacaswell commented Nov 14, 2016

Uh oh!

anntzer commented Nov 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Nov 14, 2016

Uh oh!

anntzer commented Nov 14, 2016

Uh oh!

tacaswell commented Nov 14, 2016

Uh oh!

dopplershift commented Nov 14, 2016

Uh oh!

QuLogic Nov 14, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

QuLogic Nov 14, 2016 •

edited

Loading