X Tutup
Skip to content

FIX: Only render single patch for scatter#7214

Merged
NelleV merged 1 commit intomatplotlib:masterfrom
tacaswell:multiple-legend-handles
Oct 18, 2016
Merged

FIX: Only render single patch for scatter#7214
NelleV merged 1 commit intomatplotlib:masterfrom
tacaswell:multiple-legend-handles

Conversation

@tacaswell
Copy link
Copy Markdown
Member

@tacaswell tacaswell commented Oct 3, 2016

Alternative to #6883

@tacaswell tacaswell added this to the 2.0 (style change major release) milestone Oct 3, 2016
NelleV
NelleV reviewed Oct 11, 2016
View reviewed changes
Copy link
Copy Markdown
Member

@NelleV NelleV left a comment

Choose a reason for hiding this comment

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

There are parts of matplotlib I just don't understand…
Sorry, my review isn't very useful: I am just trying to understand what's going on: I don't see how your patch is fixing the bug.

lib/matplotlib/legend_handler.py
@@ -316,7 +316,7 @@ def get_sizes(self, legend, orig_handle,
numpoints = self.get_numpoints(legend)
if numpoints < 4:
sizes = [.5 * (size_max + size_min), size_max,
Copy link
Copy Markdown
Member

@NelleV NelleV Oct 11, 2016

Choose a reason for hiding this comment

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

That could easily be unit tested.

Copy link
Copy Markdown
Member Author

@tacaswell tacaswell Oct 11, 2016

Choose a reason for hiding this comment

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

It is possible that this is the only change that really matters here.

Copy link
Copy Markdown
Member

@NelleV NelleV Oct 11, 2016

Choose a reason for hiding this comment

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

all the more reason to unit test it, no?
It is a very easy unit test to check that the return lengths is equal to numpoints.

Also, my two cents is that I would much prefer the returned size to be sorted (only matters for numpoints == 3)

Copy link
Copy Markdown
Contributor

@anntzer anntzer Oct 11, 2016

Choose a reason for hiding this comment

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

I also vote for drawing the markers in increasing size. (This also makes the code simpler; now only numpoints=1 needs to be special-cased.)

lib/matplotlib/backend_bases.py Outdated
return 0
Npath_ids = max(Npaths, len(all_transforms))
N = max(Npath_ids, len(offsets))
N = max(Npaths, len(offsets))
Copy link
Copy Markdown
Member

@NelleV NelleV Oct 11, 2016

Choose a reason for hiding this comment

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

If I am not mistaken, this function is only used to compute whether or not to do an optimization. Hence, over estimating the number of paths drawn is not that important and I am assuming that this as no impact on the bug you are fixing?

Copy link
Copy Markdown
Contributor

@anntzer anntzer Oct 11, 2016

Choose a reason for hiding this comment

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

(Also, if we put a fix there, I'd also just replace the next line by return int(np.ceil(N / Npath_ids)) which is much clearer in intent.)

Copy link
Copy Markdown
Member Author

@tacaswell tacaswell Oct 12, 2016

Choose a reason for hiding this comment

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

Is this even needed to fix the problem?

Copy link
Copy Markdown
Contributor

@anntzer anntzer Oct 12, 2016

Choose a reason for hiding this comment

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

It's just an optimization (see docstring) and it doesn't even really matter whether this is correct or not (modulo an unnecessary increase in the size of the svg).

@efiring
Copy link
Copy Markdown
Member

efiring commented Oct 11, 2016

The difference in the test_legend png baseline plots is imperceptible by eye. Perhaps it would be worthwhile to modify that test to include the alpha and size variation from the scatter example that showed the problem in the first place.

@NelleV
Copy link
Copy Markdown
Member

NelleV commented Oct 11, 2016

After reviewing with @anntzer today, I finally understand what's going on.
(@anntzer I am not reproducing the bug we noticed on your computer).

@anntzer
Copy link
Copy Markdown
Contributor

anntzer commented Oct 11, 2016

@NelleV I figured it out: that's because I used range(10) for sizes, which means that the smallest marker has size zero... putting a nonzero smallest size fixes the issue we were looking at.

@NelleV
Copy link
Copy Markdown
Member

NelleV commented Oct 11, 2016

Interesting. Maybe the code should raise a warning?

@anntzer
Copy link
Copy Markdown
Contributor

anntzer commented Oct 12, 2016

@NelleV that's a bit a side issue but anyways... Currently scatter mostly lets negative sizes through, a warning is simply given by numpy when square-rooting them:

In [1]: plt.scatter(range(10), range(10), -np.arange(10))
/usr/lib/python3.5/site-packages/matplotlib/collections.py:862: RuntimeWarning: invalid value encountered in sqrt
  scale = np.sqrt(self._sizes) * dpi / 72.0 * self._factor
Out[1]: <matplotlib.collections.PathCollection at 0x7fa0f18e3dd8>

(resulting plot is empty).

If anything this should raise a ValueError (and whether to error on zero sizes or not is a separate issue).

@NelleV NelleV changed the title FIX: Only render single patch for scatter [MRG+1] FIX: Only render single patch for scatter Oct 12, 2016
@NelleV NelleV mentioned this pull request Oct 13, 2016
Merged
@QuLogic
Copy link
Copy Markdown
Member

QuLogic commented Oct 16, 2016

Ping @mdboom for thoughts vs. #6883.

@tacaswell tacaswell force-pushed the multiple-legend-handles branch from 2830f11 to 524cfbd Compare October 17, 2016 19:13
@NelleV
Copy link
Copy Markdown
Member

NelleV commented Oct 18, 2016

The failure seems unrelated, but is consistently here…

@tacaswell
Copy link
Copy Markdown
Member Author

tacaswell commented Oct 18, 2016

.... and only on pytest.

When in doubt reboot rebase

@tacaswell tacaswell force-pushed the multiple-legend-handles branch from 524cfbd to 25fbe8f Compare October 18, 2016 02:15
@NelleV
Copy link
Copy Markdown
Member

NelleV commented Oct 18, 2016

Thanks @tacaswell !

@NelleV NelleV merged commit 9ee2d29 into matplotlib:master Oct 18, 2016
@tacaswell tacaswell deleted the multiple-legend-handles branch October 18, 2016 16:18
tacaswell pushed a commit that referenced this pull request Oct 18, 2016
@NelleV @tacaswell
Merge pull request #7214 from tacaswell/multiple-legend-handles
568b523 
FIX: Only render single patch for scatter
@tacaswell
Copy link
Copy Markdown
Member Author

tacaswell commented Oct 18, 2016

backported to v2.x as 568b523

@QuLogic QuLogic changed the title [MRG+1] FIX: Only render single patch for scatter FIX: Only render single patch for scatter Oct 18, 2016
@QuLogic QuLogic mentioned this pull request Oct 18, 2016
Closed
@QuLogic QuLogic mentioned this pull request Oct 25, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anntzer anntzer anntzer left review comments

@NelleV NelleV NelleV approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@tacaswell @efiring @NelleV @anntzer @QuLogic @mdboom
X Tutup