Sticky margins #7476

anntzer · 2016-11-17T06:12:32Z

Let artists register lists of "sticky" x and y values. If an edge of
the union of bounding boxes ends up falling on a sticky value, do not
add margins to that edge.

Docs still missing.

See #7471 (comment).

I may not have so much time to work on this in the coming days, feel free to directly push to my repo to update this PR.

TODO list

allow manual disabling of sticky edges
document how to get old style style back.

efiring

This looks very promising. I'm a little uncomfortable with the name stickies, but I don't have a better suggestion yet. I really like the reduction in LOC.

efiring · 2016-11-17T07:13:42Z

lib/matplotlib/axes/_axes.py

+            x_stickies, y_stickies = r.get_stickies()
+            (getattr(r.get_stickies(),
+                     {'vertical': 'y', 'horizontal': 'x'}[orientation])
+             .append(0))


It looks like you have a 4-physical-line obfuscation of the following 5 lines, correct?

x_stickies, y_stickies = r.get_stickies() if orientation == 'vertical': y_stickies.append(0) else: x_stickies.append(0)

But you only need x or y, so you could also do

if orientation == `vertical`: r.get_stickies().y.append(0) else: r.get_stickies().x.append(0)

so we are back down to 4 physical lines that I find much easier to understand. And it would look even nicer with stickies as a property.

efiring · 2016-11-17T07:15:07Z

lib/matplotlib/artist.py

@@ -926,98 +928,13 @@ def set_zorder(self, level):
        self.pchanged()
        self.stale = True

-    def get_top_margin(self):
+    def get_stickies(self):


I suggest making stickies a read-only property.

efiring · 2016-11-17T07:21:43Z

lib/matplotlib/axes/_axes.py

+            for patch in patches:
+                (getattr(patch.get_stickies(),
+                         {'vertical': 'y', 'horizontal': 'x'}[orientation])
+                 .append(0))


I suggest de-obfuscation as in the previous example.

anntzer · 2016-11-17T08:14:15Z

@efiring I have addressed all your comments (which I agree with, in particular making stickies a readonly property.

The only artist which I haven't handled yet is contour (and thus the related tests should fail). I am looking for a way to retrieve the extents of the contour, other than reusing the logic from QuadContourSet._initialize_x_y (I could store the variables computed at the end of _process_args for later use (self.collections isn't populated, or even created, yet, at that point), but perhaps I'm missing something less awkward?). Any ideas?

efiring · 2016-11-17T17:33:44Z

This is a good example of why the ContourSet probably should be a compound artist, added to the Axes as a unit, instead of a bunch of collections that are independently added to the Axes.

Given that now is not the time for that major refactoring, I think the thing to do is to save the data limits where they are calculated at the end of each implementation of _process_args, use them to set the stickies for each collection where it is added to the Axes, and move the setting of the data limits and the call to autoscale_view(tight=True) to the end of the __init__ methods.

efiring · 2016-11-17T21:11:13Z

numpy.isclose is new in 1.7, and we still require 1.6. #7396. Sure wish we could just do it now.

QuLogic · 2016-11-17T21:21:36Z

np.allclose is probably a good enough replacement.

efiring · 2016-11-17T21:27:05Z

No, it won't work in this case. The condition is np.any(np.isclose(a, b)).

anntzer · 2016-11-17T21:59:55Z

It looks like the tests are passing now, with the exception of numpy 1.6 (due to isclose) and docs (which I haven't touched at all).

(As a side note, perhaps we should think briefly whether it should be isclose(x0, stickies) or isclose(stickies, x0) (check the docstring to see the difference).)

Realistically, I should probably be working on other stuff right now, so I would appreciate if someone could pick up the PR from there. If not I should be able to get back to it by the end of the month.

efiring · 2016-11-17T22:07:07Z

Thanks for bringing it this far in a very short time! I expect that between @tacaswell and myself (and anyone else who is interested) we can pick it up, if Tom thinks this is the way to go, and we don't find some blocking problem with it.

efiring · 2016-11-18T18:41:34Z

Closing and reopening to restart the tests after merge of #7483.

codecov-io · 2016-11-20T17:24:49Z

Current coverage is 61.90% (diff: 90.90%)

Merging #7476 into master will decrease coverage by 0.09%

@@             master      #7476   diff @@
==========================================
  Files           173        173          
  Lines         56004      56103    +99   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          34724      34731     +7   
- Misses        21280      21372    +92   
  Partials          0          0

Powered by Codecov. Last update 9bcbfc4...360dba7

tacaswell · 2016-11-20T22:41:51Z

I am a bit confused why these tests are only failing with pytest and not with nose.

tacaswell · 2016-11-20T22:44:55Z

I get legit failures locally now (not unbound local).

anntzer · 2016-11-21T00:34:32Z

The failure comes from a faulty calculation of interval.dataLim in the stacked stepfilled case of hist (line 6441 before this PR, self.dataLim.intervaly = (ymin, ymax)). This faulty calculation was present even as of 1.5.3 and can be seen e.g. by calling autoscale after a stacked stepfilled plot:

hist(np.random.rand(100, 2), stacked=True, histtype="stepfilled"); autoscale()

(which basically only autoscales based on the upper stacked histogram).
(You can also print the value of gca().dataLim at that point.)

This issue was not visible before because the previous implementation did not rely on autoscaling at that point (I think it relied on the calls to autoscale during the creation of the individual artists that composed the stacked histogram). Now that we call autoscale_view at the end of hist(), the bug gets exposed.

QuLogic · 2016-11-24T05:35:39Z

Is that perhaps related to #4414?

anntzer · 2016-11-24T05:42:07Z

Likely yes.

anntzer · 2016-11-24T07:42:42Z

lib/matplotlib/axes/_axes.py

-                xmin = max(xmin*0.9, minimum) if not input_empty else minimum
-                xmin = min(xmin0, xmin)
-                self.dataLim.intervalx = (xmin, xmax)
+                self.dataLim.intervalx = (0, xmax)


This would fail with negative weights, or if there are already other artists present with greater extents.

Rats! The idea of negative weights and negative histogram values never would have occurred to me; it can occur mathematically, but it doesn't make sense for what I think of as a "histogram".

The original code was broken, even before 2.x; the present code is broken. It turns out that the present "sticky" algorithm is not doing the right thing for plain bar plots, either, when switching to a log scale; this was working in 1.5.3. I don't know if it was working with the "margins" algorithm in 2.x.

log-scale bar histograms seem to work fine for me (np.random.seed(0); plt.hist(np.random.randn(1000), 10, log=True) gives an identical plot with 2.0b4 and with this branch), so at least in theory it should also be possible to make it work correctly with stepfilled plots?

Yes, everything can be made to work, at least under reasonable circumstances... but the devil is in the details. I am making progress but running into problems with the test framework. Rebasing on current master helps, I think. Do you mind if I force-push the rebase?

Not at all, go ahead.

Let artists register lists of "sticky" x and y values. If an edge of the union of bounding boxes ends up falling on a sticky value, do not add margins to that edge. Docs still missing.

efiring · 2016-11-29T06:57:06Z

Now there is still one appveyor test failure (only in the 2.7 "test all" run):

ImageComparisonFailure: images not close: C:\projects\matplotlib\result_images\test_png\pngsuite.png vs. C:\projects\matplotlib\result_images\test_png\pngsuite-expected.png (RMS 0.011)

tacaswell · 2016-11-29T13:53:15Z

That one also needs to have it's thershold bumped, but is also failing on master (unrelated to this).

efiring · 2016-11-29T18:55:17Z

After yesterday's conversation, I think that we need to add one more component: an API for switching the sticky_edges processing on or off in the autoscaling. The reason is that the sticky_edges machinery is an attempt to provide behavior that matches the type of artist being plotted, and to do a reasonable job of handling a mixture of artists. There will inevitably be cases where the machinery doesn't automatically do what the user wants, however, so there needs to be an easy way of saying "ignore the sticky_edges and use the specified margins". At present the user could do this by emptying the sticky_edges lists prior to a final call to autoscale_view, but this is too fiddly, and it involves erasing information that probably should be preserved, so that the operation is reversible. Instead, we probably need something like an Axes._use_sticky_edges flag, with a simple API for manipulating it.
I can add that some time within the next few days.

anntzer · 2016-11-30T05:49:40Z

See also #6281 (comment) re: reversibility.

NelleV · 2016-12-01T18:23:14Z

lib/matplotlib/axes/_base.py

+            stickies = [artist.sticky_edges for artist in self.get_children()]
+            x_stickies = sum([sticky.x for sticky in stickies], [])
+            y_stickies = sum([sticky.y for sticky in stickies], [])
+            if self.get_xscale().lower() == 'log':


Shouldn't the presence of negative stickies with log scale raise an error?

Not any more than negative values in a plot do, I think?

Well, I think it is a bug that we tolerate this :)

@NelleV, you think that if a histogram plot is switched to a log scale it should fail rather than yielding a plot? In other words, simply bumping the "L" key when a histogram plot is active should raise an exception?

If it means that the following code raises an error, then yes I think it should.
I think the fact that matplotlib accepts log scale when the user provides negative value is at high risk of having unintended bugs, and leads to wrong results and interpretation of analysis.
On the other hand, I'd be fine if the error was caught internally when in interactive mode.

import matplotlib.pyplot as plt import numpy as np fig, ax = plt.subplots() ax.set_yscale("log") ax.plot(np.arange(0, 10)) ax.plot(np.arange(-100, 0))

I think the behavior proposed by @NelleV is reasonable but this should not be handled by the sticky system (nor block this PR) -- it should affect all plots, including those who do not set sticky margins.

tacaswell · 2016-12-02T23:24:45Z

lib/matplotlib/axes/_base.py

+
+    @use_sticky_edges.setter
+    def use_sticky_edges(self, b):
+        self._use_sticky_edges = bool(b)


Should this also re-trigger an auto-scaling?

I was wondering about that also, but decided against it. I could be convinced otherwise for now, but long term I think we might want to consolidate the autoscaling at draw time instead of having calls to autoscale_view sprinkled all over the place. I don't think it makes sense to trigger autoscaling with every change in the state of the plot.

I am convinced

Do we actually need the axes as stale? This won't have an effect until the next autoscaling, which should also mark it as stale, right?

Good point. You are welcome to replace it with a comment to that effect. I think I put the call in because we tend to have it in every function that potentially changes how the plot will look.

Done, together with indentation fix below.

Not sure I agree with this, if as Eric suggests above we move towards autoscaling a draw time (as late as possible) then marking the figure as stale will trigger a re-draw (and hence the re-autoscaling).

Good point.
On the other hand this is easy enough to revert when we do switch to draw-time autoscaling (which makes complete sense).

efiring · 2016-12-03T06:08:53Z

I've bumped this back to "MRG+1" based on Tom's earlier approval and all tests passing. If people are happy enough with the API, including the Axes.use_sticky_edges property that I added, then I recommend merging without waiting for all documentation additions and fixups. We need to get people started using it so we can see whether it will require any more changes prior to v2.0.

QuLogic · 2016-12-03T08:27:28Z

lib/matplotlib/axes/_axes.py

-                                    **kwargs)
+            if style == "image":
+                im = mimage.AxesImage(self, cmap, norm,
+                                            interpolation='nearest',


I guess it passes, but it's a bit of a weird indent.

True. It's accidental. You are welcome to fix it.

tacaswell · 2016-12-03T16:32:46Z

lib/matplotlib/tri/tricontour.py

-            x1 = tri.x.max()
-            y0 = tri.y.min()
-            y1 = tri.y.max()
-            self.ax.update_datalim([(x0, y0), (x1, y1)])


I do not think we need the line below this either, the autoscaling gets called later in the init.

tacaswell · 2016-12-03T16:40:05Z

Assuming my most recent commit passes, I agree with @efiring that this should be merged.

Given that @anntzer and @efiring both have large numbers of commit is this, can @NelleV or @QuLogic give this a final review and push the green button?

NelleV · 2016-12-04T18:46:30Z

Green button pushed.

NelleV · 2016-12-04T18:47:40Z

On the other hand, related tickets have probably not been closed. Can someone elte take care of this?

tacaswell · 2016-12-04T20:54:16Z

I am in the process of backporting this

Sticky margins Conflicts: lib/matplotlib/streamplot.py - discard conflicting diff, it is on changes to code which is not backported to 2.x lib/matplotlib/tests/test_triangulation.py - discard win-specific test changes (they do not exist on 2.x) lib/matplotlib/tests/baseline_images/test_transforms/pre_transform_data.pdf lib/matplotlib/tests/baseline_images/test_transforms/pre_transform_data.svg - kept version from master

tacaswell · 2016-12-04T21:48:31Z

backported to v2.x as 0289b4f

anntzer force-pushed the sticky-margins branch from 832884e to 231399c Compare November 17, 2016 06:13

efiring reviewed Nov 17, 2016

View reviewed changes

anntzer force-pushed the sticky-margins branch from 231399c to f8bd0b4 Compare November 17, 2016 08:08

anntzer force-pushed the sticky-margins branch from f4f9e75 to fdc07a0 Compare November 17, 2016 20:02

anntzer force-pushed the sticky-margins branch from fdc07a0 to 355de7b Compare November 17, 2016 21:35

efiring mentioned this pull request Nov 17, 2016

Bump numpy minimal version to 1.7.0? #7396

Closed

efiring closed this Nov 18, 2016

efiring reopened this Nov 18, 2016

efiring added this to the 2.0 (style change major release) milestone Nov 23, 2016

QuLogic added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Nov 24, 2016

anntzer commented Nov 24, 2016

View reviewed changes

anntzer and others added 5 commits November 24, 2016 11:24

Minor cleanups.

b73a939

Sticky margins cancellation system.

a2e35cc

Let artists register lists of "sticky" x and y values. If an edge of the union of bounding boxes ends up falling on a sticky value, do not add margins to that edge. Docs still missing.

Sticky margins support for contour.

0fa187e

Update test images.

e646152

DOC: remove docs for removed API

5d82bcb

test_triangulation: increase tolerance on Windows

840986e

efiring changed the title ~~[MRG] Sticky margins~~ [WIP] Sticky margins Nov 29, 2016

NelleV reviewed Dec 1, 2016

View reviewed changes

NelleV and others added 2 commits December 1, 2016 10:39

DOC sticky edges now uses numpydoc

1a86b08

add ax.use_sticky_edges property

04298b8

efiring mentioned this pull request Dec 2, 2016

scatter: disable the margin override except in classic mode #7557

Merged

tacaswell reviewed Dec 2, 2016

View reviewed changes

efiring changed the title ~~[WIP] Sticky margins~~ [MRG+1] Sticky margins Dec 3, 2016

QuLogic reviewed Dec 3, 2016

View reviewed changes

tacaswell reviewed Dec 3, 2016

View reviewed changes

MNT: remove extraneous autoscale call

93c1bd8

Remove extraneous stale setting; fix indent.

360dba7

NelleV merged commit 6223155 into matplotlib:master Dec 4, 2016

This was referenced Dec 4, 2016

new margin system doesn't handle negative values in bars #7175

Closed

Questionable margin-cancellation logic #6565

Closed

anntzer deleted the sticky-margins branch December 4, 2016 21:56

QuLogic changed the title ~~[MRG+1] Sticky margins~~ Sticky margins Dec 4, 2016

This was referenced Dec 11, 2016

Specifying histtype='stepfilled' and normed=True when using plt.hist causes ymax to be set incorrectly #4414

Closed

Fix issue #4414 #6171

Closed

Sticky margins #7476

Sticky margins #7476

Conversation

anntzer commented Nov 17, 2016 • edited by NelleV Loading

efiring left a 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

anntzer commented Nov 17, 2016

efiring commented Nov 17, 2016

efiring commented Nov 17, 2016

QuLogic commented Nov 17, 2016

efiring commented Nov 17, 2016

anntzer commented Nov 17, 2016

efiring commented Nov 17, 2016

efiring commented Nov 18, 2016

codecov-io commented Nov 20, 2016 • edited Loading

Current coverage is 61.90% (diff: 90.90%)

tacaswell commented Nov 20, 2016

tacaswell commented Nov 20, 2016

anntzer commented Nov 21, 2016 • edited Loading

QuLogic commented Nov 24, 2016

anntzer commented Nov 24, 2016

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

efiring commented Nov 29, 2016

tacaswell commented Nov 29, 2016

efiring commented Nov 29, 2016

anntzer commented Nov 30, 2016

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiring commented Dec 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Dec 3, 2016

NelleV commented Dec 4, 2016

NelleV commented Dec 4, 2016

tacaswell commented Dec 4, 2016

tacaswell commented Dec 4, 2016

anntzer commented Nov 17, 2016 •

edited by NelleV

Loading

codecov-io commented Nov 20, 2016 •

edited

Loading

anntzer commented Nov 21, 2016 •

edited

Loading