FIX: array_view construction for empty arrays #5106

jkseppan · 2015-09-20T15:50:01Z

Fixes #5105. The logic gets a little bit complicated, since a lot of tests started failing at first. The last commit accepts e.g. an array of shape (0,) when expecting a three-dimensional array, which should be safe since there is no data in the array anyway, and this case does happen in our code.

WeatherGod · 2015-09-21T15:57:48Z

Huh... Travis hasn't been invoked...

jkseppan · 2015-09-21T15:59:00Z

Must be the recent AWS outage. Perhaps if I make some small change to the commits?

Unless ND==0, in which case return success but don't clobber the m_shape and m_strides members.

jkseppan · 2015-09-21T16:14:14Z

Seems to be failing in svg tests. I don't have the prerequisites on my system, so the tests were skipped as KNOWNFAIL.

tacaswell · 2015-09-21T16:19:58Z

attn @mdboom

tacaswell · 2015-09-24T02:06:37Z

Tests pass and nothing seems wrong, but I have no experience with these parts of the code. Who other than @mdboom can review this?

efiring · 2015-09-26T21:50:35Z

Maybe @ianthomas23? Or @njsmith?

njsmith · 2015-09-26T22:10:23Z

I've looked at the comment thread and the patch, and I still have no idea
what this is about :-).
.
The comment about ignoring shape mismatches for empty arrays seems very odd
(why would you want to do that?), and the way the code seems to define
"empty" as equivalent to "arr.shape[0] == 0" is extremely odd. (Normally
empty means "0 in arr.shape", or there are probably better ways to check
via the C api but I'd have to go check the docs).
On Sep 26, 2015 2:50 PM, "Eric Firing" notifications@github.com wrote:

Maybe @ianthomas23 https://github.com/ianthomas23? Or @njsmith
https://github.com/njsmith?

—
Reply to this email directly or view it on GitHub
#5106 (comment)
.

jkseppan · 2015-09-27T04:52:34Z

I agree that arr.shape[0] == 0 only catches a subset of all empty arrays, but that's what was in the code before and that's what other code implicitly assumes to be allowed. As to the reason you might want to do that, I think this is so that you can have a 2D view and have the caller pass in [], since that's given a shape of (0,) by numpy. Normally at Python level that's handled by broadcasting.

njsmith · 2015-09-27T05:05:20Z

I guess I will fall back on my default method of sounding like I know something in a code review.

Could you add some tests?

Instead change callers to have empty arrays of the right shape.

jkseppan · 2015-09-27T06:44:32Z

I think we can do without allowing shape mismatches, it just means a few changes to the calling code.

We don't have C++ tests yet and I hesitate to add a test framework for a bugfix commit close to a release.

njsmith · 2015-09-27T07:03:52Z

#5105 has a good test if nothing else

WeatherGod · 2015-09-28T13:40:14Z

lib/matplotlib/cbook.py

@@ -2244,6 +2244,21 @@ def _reshape_2D(X):
    return X


+def ensure_3d(arr):


Do we want to make this a public function? We now have _check_1d(), _reshape_2D, and this new ensure_3d (notice the inconsistency with the case of "d/D").

Why don't we make this private, and make a new issue to unify the logic across these three functions later (if possible)?

I would have thought that an underscore in the beginning means a function that is only called from within cbook, but clearly some underscored functions are called from elsewhere. What is the intended meaning?

https://www.python.org/dev/peps/pep-0008/#id29

It means that it is to be treated as being for internal use only. This
gives developers the freedom to modify the API of such modules or functions
without warning or deprecation cycles. As noted later in PEP8, it is easier
to start with something as private and then make it public later, as
opposed to releasing it as public and then modifying it later.

On Mon, Sep 28, 2015 at 10:23 AM, Jouni K. Seppänen <
notifications@github.com> wrote:

In lib/matplotlib/cbook.py
#5106 (comment):

@@ -2244,6 +2244,21 @@ def _reshape_2D(X):
return X

+def ensure_3d(arr):

I would have thought that an underscore in the beginning means a function
that is only called from within cbook, but clearly some underscored
functions are called from elsewhere. What is the intended meaning?

—
Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/5106/files#r40557282.

PEP8 doesn't really say what the boundaries of internal use are, but I guess here it means internal to matplotlib. Ok, I'll change this.

It's not exactly the same, but we really use it just for empty arrays, so it shouldn't actually matter.

jkseppan · 2015-09-28T15:53:51Z

On second thoughts, ensure_3d was unnecessary. I thought I couldn't use numpy.atleast_3d because of how it adds dimensions, but in fact only the dimensionality of empty arrays was ever affected. The calls that caused test failures were related to constructs like [np.array(x) for x in bboxes], which becomes a 1d empty array when bboxes is empty. But with empty arrays it doesn't actually matter what the exact shape is, since the data will never be iterated over.

WeatherGod · 2015-09-28T16:16:27Z

If that's the case, then are we also able to do away with the changes in collections.py?

jkseppan · 2015-09-28T17:06:06Z

No. [] is a one-dimensional empty array, but the C++ code it gets passed to expects three dimensions. As an alternative, this PR as of several commits ago allowed a dimension mismatch for empty arrays at the C++ level, but that was considered "odd" in review.

WeatherGod · 2015-09-28T17:22:35Z

ok, makes sense. From the python side of things, this looks good. I can't really speak for the C++ side, but I don't really see anything troubling.

tacaswell · 2015-09-28T17:40:27Z

If @mdboom does not weigh in by this evening I will merge.

pelson · 2015-10-01T09:04:42Z

lib/matplotlib/collections.py

-    _transforms = []
-
-
+    _transforms = np.empty((0, 3, 3))


Since you've touched it, would you mind adding a #: style docstring for what _transforms is supposed to be - this will make reviewing (and editing) this part of the code easier in the future.

Where is this #: style documented?

I think @pelson just means to add a normal comment explaining what the expected shape of _transfroms is and why?

I see with git grep that there is a small number of #: comments in the code, but that's not a very easy thing to search. Does Sphinx do something special with them, or some other tool?

Indeed it does: http://sphinx-doc.org/ext/autodoc.html#directive-autoattribute

FIX: array_view construction for empty arrays

tacaswell · 2015-10-02T02:47:09Z

Thanks folks.

jkseppan mentioned this pull request Sep 20, 2015

1.5rc1: Calling figure transform with wrong argument crashs python #5105

Closed

jkseppan added the status: needs review label Sep 20, 2015

jkseppan added 3 commits September 21, 2015 18:59

Return error from array_view for numpy scalars

e6b1745

Unless ND==0, in which case return success but don't clobber the m_shape and m_strides members.

Catch errors in the 1d branch of Py_affine_transform

724cb9b

Accept dimension mismatch for empty arrays

2f29bdf

jkseppan force-pushed the 0d-views branch from 33178e9 to 2f29bdf Compare September 21, 2015 15:59

Assign m_arr = tmp in more cases

c0e09d5

tacaswell added this to the next point release (1.5.0) milestone Sep 26, 2015

tacaswell mentioned this pull request Sep 26, 2015

FIX: set internal flags first in FigureCanvasBase #5150

Merged

Don't allow shape mismatches for empty arrays

536d175

Instead change callers to have empty arrays of the right shape.

Add test

3965858

WeatherGod reviewed Sep 28, 2015
View reviewed changes

Use atleast_3d instead of ensure_3d

2f5a633

It's not exactly the same, but we really use it just for empty arrays, so it shouldn't actually matter.

pelson reviewed Oct 1, 2015
View reviewed changes

Add a comment about _transforms

123deb5

tacaswell added a commit that referenced this pull request Oct 2, 2015

Merge pull request #5106 from jkseppan/0d-views

15b8629

FIX: array_view construction for empty arrays

tacaswell merged commit 15b8629 into matplotlib:master Oct 2, 2015

tacaswell removed the status: needs review label Oct 2, 2015

jkseppan deleted the 0d-views branch October 2, 2015 04:03

tacaswell mentioned this pull request Oct 13, 2015

Random test failures in Legent tests (1.5.0rc2) #5185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: array_view construction for empty arrays #5106

FIX: array_view construction for empty arrays #5106

jkseppan commented Sep 20, 2015

WeatherGod commented Sep 21, 2015

jkseppan commented Sep 21, 2015

jkseppan commented Sep 21, 2015

tacaswell commented Sep 21, 2015

tacaswell commented Sep 24, 2015

efiring commented Sep 26, 2015

njsmith commented Sep 26, 2015

jkseppan commented Sep 27, 2015

njsmith commented Sep 27, 2015

jkseppan commented Sep 27, 2015

njsmith commented Sep 27, 2015

WeatherGod Sep 28, 2015

jkseppan Sep 28, 2015

WeatherGod Sep 28, 2015

jkseppan Sep 28, 2015

jkseppan commented Sep 28, 2015

WeatherGod commented Sep 28, 2015

jkseppan commented Sep 28, 2015

WeatherGod commented Sep 28, 2015

tacaswell commented Sep 28, 2015

pelson Oct 1, 2015

jkseppan Oct 1, 2015

tacaswell Oct 1, 2015

jkseppan Oct 1, 2015

tacaswell Oct 1, 2015

tacaswell commented Oct 2, 2015

		@@ -2244,6 +2244,21 @@ def _reshape_2D(X):
		return X


		def ensure_3d(arr):

FIX: array_view construction for empty arrays #5106

FIX: array_view construction for empty arrays #5106

Conversation

jkseppan commented Sep 20, 2015

WeatherGod commented Sep 21, 2015

jkseppan commented Sep 21, 2015

jkseppan commented Sep 21, 2015

tacaswell commented Sep 21, 2015

tacaswell commented Sep 24, 2015

efiring commented Sep 26, 2015

njsmith commented Sep 26, 2015

jkseppan commented Sep 27, 2015

njsmith commented Sep 27, 2015

jkseppan commented Sep 27, 2015

njsmith commented Sep 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkseppan commented Sep 28, 2015

WeatherGod commented Sep 28, 2015

jkseppan commented Sep 28, 2015

WeatherGod commented Sep 28, 2015

tacaswell commented Sep 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Oct 2, 2015