FIX: array_view construction for empty arrays

[jkseppan]

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]

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

[jkseppan]

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

[jkseppan]

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

[tacaswell]

attn @mdboom

[tacaswell]

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]

Maybe @ianthomas23? Or @njsmith?

[njsmith]

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]

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]

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?

[jkseppan]

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]

#5105 has a good test if nothing else

[WeatherGod]

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)?

[jkseppan]

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?

[WeatherGod]

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.

[jkseppan]

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.

[jkseppan]

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]

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

[jkseppan]

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]

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]

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

[pelson]

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.

[jkseppan]

Where is this #: style documented?

[tacaswell]

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

[jkseppan]

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?

[tacaswell]

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

[tacaswell]

Thanks folks.