Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix for empty collection check in axes.add_collection #1497

Merged
merged 4 commits into from

8 participants

Anton Akhmerov Paul Ivanov Damon McDougall Eric Firing Benjamin Root Michael Droettboom Varoquaux Phil Elson
Anton Akhmerov

Fixes #1490

Anton Akhmerov

collection._paths is a list, so bool(collection._paths) is the same as bool(len(collection._paths)), and the second condition in the original code is not required. On the other hand, collection._offsets is always a numpy array, so a length check is appropriate, while checking for truth value wouldn't work, and checking for other formats isn't necessary.

Paul Ivanov
Collaborator

looks good, @akhmerov, thanks for the fix. Could you add a test for this, so we don't run into it again - something like the original report in #1490?

Anton Akhmerov

Done. As I mentioned, this is not a full fix of #1490, and another issue has to be opened to ensure that datalimits of empty collections are calculated properly.

Varoquaux NelleV commented on the diff
lib/matplotlib/axes.py
@@ -1491,9 +1491,8 @@ def add_collection(self, collection, autolim=True):
if collection.get_clip_path() is None:
collection.set_clip_path(self.patch)
- if autolim:
- if collection._paths and len(collection._paths):
- self.update_datalim(collection.get_datalim(self.transData))
+ if autolim and collection._paths and len(collection._offsets):
Varoquaux Collaborator
NelleV added a note

Tiny nitpick on something you haven't written: we should not test for an emtpy list using len(list) but just list.
An empty list evaluates as false in python.

Maybe you can replace:

if autolim and collection._paths and len(collection._offsets):

by

if autolim and collection._paths and collection._offsets:

As I wrote in a comment earlier, collection._offsets is a numpy array, not a list. This means that bool(np.array([0])) == False, and bool(np.array([0, 0])) raises an error.

Varoquaux Collaborator
NelleV added a note

My mistake.

I'd then use the shape method instead of the length attribute, but that's too much nitpicking :)

Thanks for clarifying.

Behavior of len is well-defined and documented for numpy arrays, so I still believe it's the better way due to improved readability.

Benjamin Root Collaborator

I agree with @akhmerov on this point. I personally don't like the idiom of the empty list evaluating to false partly because of this reason. I work with numpy arrays so much. My second reason is that an iterator to an empty list evaluates to True, and so could cause a lot of confusion when coding in py3k with iterators being so prevelent there.

@WeatherGod Well, iterators don't necessarily have length, so some assumptions about collection._paths must be made, and it seems to be a list in all cases that occur so far. I think that without going beyond the scope of this pull request, there's no further improvement to the check. What I find a bit ugly is that both _paths and _offsets are internal to collection, and should preferably not be used from outside of collections.py .

Is there anything else to be done with this pull request?

Benjamin Root Collaborator

The more and more I look at this PR, the more I can't bring myself to accept it. I would rather fix the underlying problem.

This PR doesn't introduce yet another special condition, it rather corrects the condition which already existed before (I even suspect the original condition was a typo, since the second clause was pointless). In this sense it doesn't hurt anything. Even if path.get_path_collection_extents would be working properly, this condition could be reasonable to keep.

Benjamin Root Collaborator

The problem that can now occur is if collection._offsets is None. Previously, the check on collection._paths would be sufficient to prevent an exception from being thrown when doing a len() on None. Now that protection is gone.

Anton Akhmerov
akhmerov added a note

Line 1440 of collections.py in my fork of matplotlib uses the same assumption:

if len(self._offsets):
    xs = self.convert_xunits(self._offsets[:0])

Additionally, searching for all occurences of _offsets, I cannot confirm that they can ever assume a None value, instead _offsets are always an array.

Phil Elson Collaborator
pelson added a note

@WeatherGod / @akhmerov - I've submitted a PR which moves this line to the Collection class in v1.3.1: #2444

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Damon McDougall
Collaborator

What's the status on this? Is it good to go? The tests pass but I'm aware @WeatherGod has some concerns. How do others feel?

Eric Firing
Owner

I don't see any reason not to merge it.

Benjamin Root
Collaborator

I would be willing to back off if we make a new issue to investigate the root reason for the bug that this is papering over. We just need to figure out a way to still be able to test for that bug even with this fix.

Anton Akhmerov

This is also my suggestion (I didn't fix the underlying issue due to lacking time/cpp skill). Testing the underlying issue would be easy: one just needs to check output on path.get_path_collection_extents when supplied an empty collection, as I described in #1490.

Damon McDougall
Collaborator

Ok sounds like, as @WeatherGod said, if we open a separate issue for the underlying problem we are pretty happy to merge this PR.

@WeatherGod Would you mind opening a new issue?

Benjamin Root
Collaborator
Damon McDougall
Collaborator

I think that's a fine idea.

Anton Akhmerov

A known failure test would require knowing what is the correct return value for path.get_path_collection_extents if the collection is empty. I don't think this is set anywhere as of now.

Anton Akhmerov

Since creating a KnownFailure test would require deciding first, what should happen if the test was not to fail, and since no such action was taken, I have created #1735 to not lose track of the bug in _path.get_path_collection_extents. I think this PR can be now merged and #1490 closed.

lib/matplotlib/tests/test_axes.py
@@ -54,6 +54,18 @@ def test_formatter_ticker():
ax.set_xlabel( "x-label 005" )
ax.autoscale_view()
+def test_add_collection():
Michael Droettboom Owner
mdboom added a note

This needs to have the @cleanup decorator to the figures get cleared after the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Phil Elson pelson commented on the diff
lib/matplotlib/tests/test_axes.py
@@ -54,6 +54,19 @@ def test_formatter_ticker():
ax.set_xlabel( "x-label 005" )
ax.autoscale_view()
+@cleanup
+def test_add_collection():
Phil Elson Collaborator
pelson added a note

I think this test needs a comment re what the purpose of the test is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Eric Firing efiring merged commit 99d1a2f into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 14, 2012
  1. Anton Akhmerov
  2. Anton Akhmerov

    Add test for add_collection

    akhmerov authored
Commits on Feb 4, 2013
  1. Anton Akhmerov
Commits on Feb 14, 2013
  1. Anton Akhmerov
This page is out of date. Refresh to see the latest.
Showing with 17 additions and 3 deletions.
  1. +2 −3 lib/matplotlib/axes.py
  2. +15 −0 lib/matplotlib/tests/test_axes.py
5 lib/matplotlib/axes.py
View
@@ -1491,9 +1491,8 @@ def add_collection(self, collection, autolim=True):
if collection.get_clip_path() is None:
collection.set_clip_path(self.patch)
- if autolim:
- if collection._paths and len(collection._paths):
- self.update_datalim(collection.get_datalim(self.transData))
+ if autolim and collection._paths and len(collection._offsets):
Varoquaux Collaborator
NelleV added a note

Tiny nitpick on something you haven't written: we should not test for an emtpy list using len(list) but just list.
An empty list evaluates as false in python.

Maybe you can replace:

if autolim and collection._paths and len(collection._offsets):

by

if autolim and collection._paths and collection._offsets:

As I wrote in a comment earlier, collection._offsets is a numpy array, not a list. This means that bool(np.array([0])) == False, and bool(np.array([0, 0])) raises an error.

Varoquaux Collaborator
NelleV added a note

My mistake.

I'd then use the shape method instead of the length attribute, but that's too much nitpicking :)

Thanks for clarifying.

Behavior of len is well-defined and documented for numpy arrays, so I still believe it's the better way due to improved readability.

Benjamin Root Collaborator

I agree with @akhmerov on this point. I personally don't like the idiom of the empty list evaluating to false partly because of this reason. I work with numpy arrays so much. My second reason is that an iterator to an empty list evaluates to True, and so could cause a lot of confusion when coding in py3k with iterators being so prevelent there.

@WeatherGod Well, iterators don't necessarily have length, so some assumptions about collection._paths must be made, and it seems to be a list in all cases that occur so far. I think that without going beyond the scope of this pull request, there's no further improvement to the check. What I find a bit ugly is that both _paths and _offsets are internal to collection, and should preferably not be used from outside of collections.py .

Is there anything else to be done with this pull request?

Benjamin Root Collaborator

The more and more I look at this PR, the more I can't bring myself to accept it. I would rather fix the underlying problem.

This PR doesn't introduce yet another special condition, it rather corrects the condition which already existed before (I even suspect the original condition was a typo, since the second clause was pointless). In this sense it doesn't hurt anything. Even if path.get_path_collection_extents would be working properly, this condition could be reasonable to keep.

Benjamin Root Collaborator

The problem that can now occur is if collection._offsets is None. Previously, the check on collection._paths would be sufficient to prevent an exception from being thrown when doing a len() on None. Now that protection is gone.

Anton Akhmerov
akhmerov added a note

Line 1440 of collections.py in my fork of matplotlib uses the same assumption:

if len(self._offsets):
    xs = self.convert_xunits(self._offsets[:0])

Additionally, searching for all occurences of _offsets, I cannot confirm that they can ever assume a None value, instead _offsets are always an array.

Phil Elson Collaborator
pelson added a note

@WeatherGod / @akhmerov - I've submitted a PR which moves this line to the Collection class in v1.3.1: #2444

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ self.update_datalim(collection.get_datalim(self.transData))
collection._remove_method = lambda h: self.collections.remove(h)
return collection
15 lib/matplotlib/tests/test_axes.py
View
@@ -54,6 +54,21 @@ def test_formatter_ticker():
ax.set_xlabel( "x-label 005" )
ax.autoscale_view()
+@cleanup
+def test_add_collection():
Phil Elson Collaborator
pelson added a note

I think this test needs a comment re what the purpose of the test is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ # Test if data limits are unchanged by adding an empty collection.
+ # Github issue #1490, pull #1497.
+ fig = matplotlib.figure.Figure()
+ fig2 = matplotlib.figure.Figure()
+ ax = fig.add_subplot(111)
+ ax2 = fig2.add_subplot(111)
+ coll = ax2.scatter([0, 1], [0, 1])
+ ax.add_collection(coll)
+ bounds = ax.dataLim.bounds
+ coll = ax2.scatter([], [])
+ ax.add_collection(coll)
+ assert ax.dataLim.bounds == bounds
+
@image_comparison(baseline_images=["formatter_large_small"])
def test_formatter_large_small():
# github issue #617, pull #619
Something went wrong with that request. Please try again.