Stem speedup2 #12380

dstansby · 2018-10-03T09:02:39Z

Instead of adding each line individually, add them as a line collection. This massively improves performance. Fixes #7969. This is a second go at #9565, with improved deprecation machinery.

dstansby · 2018-10-03T09:03:45Z

doc/api/next_api_changes/2017-10-24-DS.rst

+list of `Line2D` objects for stem lines plotted using `ax.stem`. This gives a
+very large performance boost to displaying and moving `ax.stem` plots.
+
+This will become the only behaviour in Matplotlib 3.3. To use it now, the


Version 3.3 is open to discussion, but I thought I'd put something in as a first go.

Do not say that it will become the only behavior in v3.3. I suggest that in v3.3, the default value for the use_line_collection keyword argument should change from False to True. Then in v3.6 we can make it a no-op (possibly emitting a warning that it is a no-op, or some other predictable behavior).

A no-op doesn't make sense. If someone explicitly uses use_line_collection=False they do it because they rely on single Lines in their code. When we remove the functionality, we also have to error out if use_line_collection != True. At that time, we can deprecate the argument itself and remove it again later on.

But generally, I agree that we should announce to change of the default in 3.3.

You are right, a no-op doesn't make sense. Certainly, we need two deprecations here to help with the transition. Yes, there will be people who won't do anything until v3.3, but at least at that point, they will update their code in a way that works for the previous two releases, and will continue to work for another release, giving a proper off-ramp.

lib/matplotlib/tests/test_axes.py

lib/matplotlib/legend_handler.py

timhoffm · 2018-10-03T15:37:25Z

I'm a bit afraid of reiterating the discussions in #9565. However, I think it's of value to describe the whole transition process:

Introduce use_line_collection defaulting to False and warn on using use_line_collection=False
Change the default to True
Remove the single line code and only support use_line_collection=True
Deprecate use_line_collection
Remove use_line_collection.

Numbers 3. and 4. might be combined.

Here's the effect on the users:

a) Lazy default user: Does not use the lines for anything. Ignores the warning.
b) Eager default user: Does not use the lines for anything. Silences the warning in 1.
c) Lines user: Works with the resulting Lines/LineCollection.

Step	lazy default user	eager default user	Lines user
1.	warned	warned, sets `use_line_collection=True`	warned, changes code, set `use_line_collection=True`
2.	-
3.	-
4.	-	warned, deletes `use_line_collection`	warned, deletes `use_line_collection`
5.	-

It's a bit unfortunate that we bother the default users (which I suppose is the majority), but I don't see a better way of handling this so that the Lines user is appropriately warned and can react before we remove the lines functionality.

dstansby · 2018-10-03T15:40:05Z

That's a nice clear explanation. With the message I've tried to give it a positive spin, so default users are motivated to use use_line_collection to increase performance.

ImportanceOfBeingErnest · 2018-10-10T20:43:42Z

I don't think steps 4. and 5. are necessary. After step 3. one can just silenetly pop use_line_collection=True out of kwargs and error out on use_line_collection=<anything other than True>.

lib/matplotlib/axes/_axes.py

anntzer · 2018-12-29T18:00:56Z

lib/matplotlib/tests/test_axes.py

+                         ids=['w/ line collection', 'w/o line collection'])
+@image_comparison(baseline_images=['stem'], extensions=['png'], style='mpl20',
+                  remove_text=True)
+def test_stem(use_line_collection):


Perhaps this may just as well use check_figures_equal to check that the result is the same regardless of use_line_collection, and avoid an additional baseline image?

I don't think ax.stem had an image test before I added this one here, so I think it's maybe worth leaving in.

Even then I'd rather check equivalency with e.g. manually creating the stem plot with individual calls to plot(), but it's just my preference.

I think it is worth having a visual test here, because it helps a lot when trying to figure out problems (especially with the legend)

Do deprecation better Fix linestyle

lib/matplotlib/axes/_axes.py

lib/matplotlib/container.py

lib/matplotlib/legend_handler.py

lib/matplotlib/axes/_axes.py

Co-Authored-By: dstansby <dstansby@gmail.com>

jklymak · 2019-02-07T17:04:54Z

lib/matplotlib/axes/_axes.py

+        else:
+            cbook._warn_external(
+                'In Matplotlib 3.3 individual lines on a stem plot will be '
+                'added as a LineCollection instead of individual lines.\n'


I don't think we have explicit carriage returns in our messages. Let the terminal take care of that...

jklymak · 2019-02-07T17:08:39Z

lib/matplotlib/legend_handler.py

-            self.update_prop(lm, m, legend)
+        if using_linecoll:
+            # update_prop() usually takes two Line2D collections;
+            # override temporarily to copy properties from a LineCollection


Found this confusing, and had to look elsewhere in code for explanation. Suggest:

"change the function used by update_prop() from the default to one that handles LineCollection"

jklymak

I had a couple of small suggestions, but this seems ready to merge

Remove carriage returns....

jklymak · 2019-02-09T21:46:34Z

@dstansby, I pushed a couple of super minor commits.

I won't merge, but someone should when CI passes

timhoffm · 2019-02-10T17:41:10Z

Flake8:
./lib/matplotlib/legend_handler.py:616:73: W291 trailing whitespace

jklymak · 2019-02-10T17:58:02Z

Flake8:
./lib/matplotlib/legend_handler.py:616:73: W291 trailing whitespace

I actually think we should not check for this. Its super annoying and really doesn't hurt the code to have an extra whitespace at the end. I understand that in 1995 wasted bits was a bad thing, but now a few extra whitespaces is not going to affect anyone

QuLogic · 2019-02-11T00:04:09Z

It was never about wasted bits, except if you count unnecessary code churn (i.e., if someone's editor has trim-whitespace on). It can cause unnecessary word wrap when windows are not too wide (e.g., side-by-side diffs), and while some people like cursor-anywhere, I prefer the cursor to only reach the end of a line where it actually logically ends.

See also the rationale in PEP8 (which has nothing to do with byte counts):

Avoid trailing whitespace anywhere. Because it's usually invisible, it can be confusing: e.g. a backslash followed by a space and a newline does not count as a line continuation marker. Some editors don't preserve it and many projects (like CPython itself) have pre-commit hooks that reject it.

timhoffm · 2019-02-15T06:29:03Z

Travis failure is unrelated. Would be fixed by a rebase to head. But saving the hassle as it passed before the final (minor) change to placate flake8.

dstansby · 2019-02-15T10:04:04Z

Hurrah, thanks for everyone who took the time to review this in its various incarnations!

dstansby added this to the v3.1 milestone Oct 3, 2018

dstansby mentioned this pull request Oct 3, 2018

Stem performance boost #9565

Closed

dstansby commented Oct 3, 2018

View reviewed changes

lib/matplotlib/tests/test_axes.py Outdated Show resolved Hide resolved

anntzer reviewed Oct 3, 2018

View reviewed changes

lib/matplotlib/legend_handler.py Outdated Show resolved Hide resolved

dstansby added the status: needs revision label Oct 3, 2018

dstansby force-pushed the stem-speedup2 branch from 15d19a4 to ec8f5f7 Compare October 4, 2018 09:04

anntzer mentioned this pull request Oct 9, 2018

FIX: make add_lines work with new colorbar #12461

Merged

6 tasks

dstansby force-pushed the stem-speedup2 branch from ec8f5f7 to ab51610 Compare October 10, 2018 15:56

dstansby added status: needs review and removed status: needs revision labels Oct 14, 2018

dstansby force-pushed the stem-speedup2 branch from 2deac6b to 829f959 Compare December 29, 2018 17:57

dstansby added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Dec 29, 2018

anntzer reviewed Dec 29, 2018

View reviewed changes

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved

anntzer reviewed Dec 29, 2018

View reviewed changes

dstansby added 2 commits January 1, 2019 16:53

Improve step performance

2098d1a

Do deprecation better Fix linestyle

Update pyplot

cdaadb8

dstansby force-pushed the stem-speedup2 branch from 829f959 to cdaadb8 Compare January 1, 2019 16:55

Fix doc tilde

00de51b

jklymak reviewed Jan 4, 2019

View reviewed changes

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved

jklymak added 2 commits January 4, 2019 09:38

...Fix warnings.warn->cbook._warn_external

be0dc01

ahem... spelling of _warn_external

a986013

timhoffm reviewed Jan 5, 2019

View reviewed changes

timhoffm and others added 4 commits January 5, 2019 21:07

Fix docstring quotes

da04fa3

Co-Authored-By: dstansby <dstansby@gmail.com>

Docstring double quotes

0bef714

Small doc change + use zip

6b19ba6

Remove code duplication

1f7228d

Fix missing indentation

6d6c786

timhoffm approved these changes Jan 19, 2019

View reviewed changes

jklymak reviewed Feb 7, 2019

View reviewed changes

jklymak approved these changes Feb 7, 2019

View reviewed changes

jklymak added 2 commits February 9, 2019 13:44

Update _axes.py

9aa02ec

Remove carriage returns....

clarified comment

7828b9c

Trailing whitespace

2e98633

timhoffm merged commit 7ca934f into matplotlib:master Feb 15, 2019

dstansby deleted the stem-speedup2 branch February 15, 2019 10:04

QuLogic removed the status: needs review label Feb 15, 2019

anntzer mentioned this pull request Sep 17, 2019

NavigationToolbar2Tk: make packing optional. #15274

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stem speedup2 #12380

Stem speedup2 #12380

dstansby commented Oct 3, 2018

dstansby Oct 3, 2018

WeatherGod Oct 3, 2018

timhoffm Oct 3, 2018 •

edited

Loading

WeatherGod Oct 3, 2018

timhoffm commented Oct 3, 2018 •

edited

Loading

dstansby commented Oct 3, 2018 •

edited

Loading

ImportanceOfBeingErnest commented Oct 10, 2018

anntzer Dec 29, 2018

dstansby Jan 1, 2019

anntzer Jan 1, 2019

dstansby Jan 19, 2019

jklymak Feb 7, 2019

jklymak Feb 7, 2019

jklymak left a comment

jklymak commented Feb 9, 2019

timhoffm commented Feb 10, 2019

jklymak commented Feb 10, 2019

QuLogic commented Feb 11, 2019

timhoffm commented Feb 15, 2019

dstansby commented Feb 15, 2019

Stem speedup2 #12380

Stem speedup2 #12380

Conversation

dstansby commented Oct 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm commented Oct 3, 2018 • edited Loading

dstansby commented Oct 3, 2018 • edited Loading

ImportanceOfBeingErnest commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak left a comment

Choose a reason for hiding this comment

jklymak commented Feb 9, 2019

timhoffm commented Feb 10, 2019

jklymak commented Feb 10, 2019

QuLogic commented Feb 11, 2019

timhoffm commented Feb 15, 2019

dstansby commented Feb 15, 2019

timhoffm Oct 3, 2018 •

edited

Loading

timhoffm commented Oct 3, 2018 •

edited

Loading

dstansby commented Oct 3, 2018 •

edited

Loading