Fix for empty collection check in axes.add_collection #1497
Conversation
|
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. |
| 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): |
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
My mistake.
I'd then use the shape method instead of the length attribute, but that's too much nitpicking :)
Thanks for clarifying.
There was a problem hiding this comment.
Behavior of len is well-defined and documented for numpy arrays, so I still believe it's the better way due to improved readability.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@WeatherGod / @akhmerov - I've submitted a PR which moves this line to the Collection class in v1.3.1: #2444
|
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? |
|
I don't see any reason not to merge it. |
|
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. |
|
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 |
|
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? |
|
Would it make sense to add a KnownFailure test to this PR and open a ticket |
|
I think that's a fine idea. |
|
A known failure test would require knowing what is the correct return value for |
| @@ -54,6 +54,18 @@ def test_formatter_ticker(): | |||
| ax.set_xlabel( "x-label 005" ) | |||
| ax.autoscale_view() | |||
|
|
|||
| def test_add_collection(): | |||
There was a problem hiding this comment.
This needs to have the @cleanup decorator to the figures get cleared after the test.
| @@ -54,6 +54,19 @@ def test_formatter_ticker(): | |||
| ax.set_xlabel( "x-label 005" ) | |||
| ax.autoscale_view() | |||
|
|
|||
| @cleanup | |||
| def test_add_collection(): | |||
There was a problem hiding this comment.
I think this test needs a comment re what the purpose of the test is.
Fix for empty collection check in axes.add_collection
Fixes #1490