10000 FIX: Only render single patch for scatter by tacaswell · Pull Request #7214 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

FIX: Only render single patch for scatter #7214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 18, 2016
Merged

FIX: Only render single patch for scatter #7214

merged 1 commit into from
Oct 18, 2016

Conversation

tacaswell
Copy link
Member

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
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
Member

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
Member Author

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
Member

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
Contributor

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
@@ -400,7 +400,7 @@ def _iter_collection_uses_per_path(self, paths, all_transforms,
if Npaths == 0 or (len(facecolors) == 0 and len(edgecolors) == 0):
return 0
Npath_ids = max(Npaths, len(all_transforms))
N = max(Npath_ids, len(offsets))
N = max(Npaths, len(offsets))
Copy link
Member

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
Contributor

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
Member Author

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
Contributor

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
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
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
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
Member
NelleV commented Oct 11, 2016

Interesting. Maybe the code should raise a warning?

@anntzer
Copy link
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
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
Member
NelleV commented Oct 18, 2016

The failure seems unrelated, but is consistently here…

@tacaswell
Copy link
Member Author

.... 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
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
Member Author

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. 6C99 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
0