Skip to content
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

Fix for empty collection check in axes.add_collection #1497

Merged
merged 4 commits into from Feb 17, 2013
Merged

Fix for empty collection check in axes.add_collection #1497

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3f0afd5
Improve empty collection check in add_collection.
akhmerov Nov 14, 2012
25b7ebd
Add test for add_collection
akhmerov Nov 14, 2012
cd777f3
add @cleanup to test_add_collection
akhmerov Feb 4, 2013
d8f3d39
Update lib/matplotlib/tests/test_axes.py
akhmerov Feb 14, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 2 additions & 3 deletions lib/matplotlib/axes.py
View file
Open in desktop
Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

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

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:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

My mistake.

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

Thanks for clarifying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

self.update_datalim(collection.get_datalim(self.transData))

collection._remove_method = lambda h: self.collections.remove(h)
return collection
Expand Down
15 changes: 15 additions & 0 deletions lib/matplotlib/tests/test_axes.py
View file
Open in desktop
Expand Up @@ -54,6 +54,21 @@ def test_formatter_ticker():
ax.set_xlabel( "x-label 005" )
ax.autoscale_view()

@cleanup
def test_add_collection():
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

# 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
Expand Down