FIX: pandas indexing error #5556

tacaswell · 2015-11-24T20:17:18Z

pd.Series prefer indexing via searching the index to positional
indexing. This method will get the first element of any iterable.

It will advance a generater, but they did not work previously anyway.

Closes #5550

has2k1 · 2015-12-12T06:22:53Z

lib/matplotlib/dates.py

@@ -1561,7 +1561,7 @@ def default_units(x, axis):
            x = x.ravel()

        try:
-            x = x[0]
+            x = next(iter(x))
        except (TypeError, IndexError):


line 1565 should be modified to

except (TypeError, StopIteration):

Thanks for catching that!

tacaswell · 2015-12-21T22:12:05Z

rebased.

tacaswell · 2015-12-22T03:59:25Z

Also addresses #5557

WeatherGod · 2015-12-28T16:56:29Z

lib/matplotlib/axes/_axes.py

+            ----------
+            inp : iterable
+            ename : str, optional
+                Name to use is ValueError if can not be normalized


"is" --> "in"?

I swear I know english...

WeatherGod · 2015-12-28T18:19:49Z

besides the nitpicking, this looks good to me

tacaswell · 2015-12-28T19:00:06Z

@WeatherGod do you want me to drop that (some what superfluous) white space commit?

WeatherGod · 2015-12-28T19:06:02Z

Eh, might be better to get rid of that commented out code entirely. I don't see the point of keeping it around.

tacaswell · 2015-12-28T19:18:14Z

Well, the minimum numpy is now 1.6 so we should do what ever it was that note was telling us to do.

WeatherGod · 2015-12-28T19:49:00Z

?

tacaswell · 2015-12-28T19:54:22Z

The comment below the commented line says that we are handling normed internally until our minimum version of numpy can handle it (which the comment claims in > 1.5). I think it will take a bit of work (at least for met) to understand how what we are currently doing works to make sure that we can just fall over to numpy.

WeatherGod · 2015-12-28T20:00:16Z

Ah, yeah, that comment (I must be getting blind...). I suspect that it is quite old and came from before the days that stacked histograms and such were possible. I am not sure it is possible to just hand off the responsibility of norming to numpy any more given the current feature set.

pd.Series prefer indexing via searching the index to positional indexing. This method will get the first element of any iterable. It will advance a generater, but they did not work previously anyway. Closes matplotlib#5550

using `next(iter(obj))` can raise `TypeError` if obj is not iterable and `StopIteration` if len(obj) == 0. As we are no longer explictily indexing into `obj` it should not raise an `IndexError`

Both iterable and index_of are imported at the top of this import block

closes matplotlib#5557

tacaswell · 2015-12-29T19:53:00Z

This is passing and not conflicting, can someone push the button on this before it needs another rebase 😈 .

WeatherGod · 2015-12-29T20:03:48Z

I do have another concern with respect to generators. Previously, generators just simply didn't work, now they may sort of work (generator gets partly consumed). This muddles the API a bit. Wouldn't it be better to detect that we have a generator and raise an error? We can later consider how to properly support generators without consuming them.

tacaswell · 2015-12-29T20:13:04Z

That is a can of worms of handling I am worried about opening and would rather punt on the whole thing. This at least does not break any existing user code.

WeatherGod · 2015-12-29T20:18:45Z

isn't it just simply a matter of testing isinstance(x, collections.Iterator)? That would be false for lists, numpy arrays and such, but should be true for generators and iterators, I think.

tacaswell · 2015-12-29T20:21:18Z

I am more worried about applying that test uniformly across the library.

WeatherGod · 2015-12-29T20:23:18Z

That would be nice, but it is not the goal that I am seeking. Simply that we add this test where-ever we have code that would potentially consume an iterator. That way, the API hasn't changed. So, that is just two places here in this PR.

tacaswell · 2015-12-29T20:23:44Z

fair enough

All versions of python we support have built in `next`

- moved next(iter(obj)) logic into a cbook function

tacaswell · 2015-12-29T20:45:52Z

Digging into this a bit (and trying to sort out why plot does not have this problem) it turns out that plot goes through an entirely different code path by using Axis.update_units which delegates to units.Registry.get_convertor which has some non-trivial logic and will do very bad things to generators (I think).

This PR is a whack-a-mole fix which will get 1.5.1 out the door.

tacaswell · 2015-12-29T21:17:36Z

The test failure is un-related (due to backport of custom RNG).

WeatherGod · 2015-12-29T21:21:19Z

seems like it, was the baseline images not updated for the new RNG?

mdboom · 2015-12-29T21:21:56Z

seems like it, was the baseline images not updated for the new RNG?

They were. This looks like a "only on Travis" sort of failure.

WeatherGod · 2015-12-29T21:24:20Z

That doesn't give me any sort of warm fuzzies...

tacaswell · 2015-12-29T21:28:56Z

I can reproduce it locally (but it passed before I pushed upstream (which I assume means my local install had become screwed up)).

WeatherGod · 2015-12-29T21:53:48Z

I hope this hasn't become another rabbit-infested dragon cave...

WeatherGod · 2015-12-30T01:47:26Z

Looks like you cleared out the rabbits!

FIX: pandas indexing error

WeatherGod · 2015-12-30T01:55:38Z

By moving a bit of the if-statement into a cbook function, the if-statement starts to be a little bit clearer. Probably would be a good idea to have some cbook tests at some point, but that can come later.

Specifically, when creating collection objects, `linewidths` and `antialiaseds` can be non zero indexed pandas series. Related to: PR #5556

This was unintentionally broken by bdaaf59 which went in as part of PR matplotlib#5556 which was fixing support for pandas series.

tacaswell added the status: needs review label Nov 24, 2015

tacaswell added this to the Critical bugfix release (1.5.1) milestone Nov 24, 2015

has2k1 reviewed Dec 12, 2015
View reviewed changes

tacaswell force-pushed the fix_pandas_indexing branch from 50b547c to 825f613 Compare December 21, 2015 22:11

WeatherGod reviewed Dec 28, 2015
View reviewed changes

tacaswell force-pushed the fix_pandas_indexing branch from 6ec1bbd to f0a8a02 Compare December 28, 2015 20:33

tacaswell added 8 commits December 28, 2015 19:01

FIX: pandas indexing error

2f73778

pd.Series prefer indexing via searching the index to positional indexing. This method will get the first element of any iterable. It will advance a generater, but they did not work previously anyway. Closes matplotlib#5550

TST: add test of bad pandas indexing

ab6d338

MNT: catch correct exceptions

af9d6eb

using `next(iter(obj))` can raise `TypeError` if obj is not iterable and `StopIteration` if len(obj) == 0. As we are no longer explictily indexing into `obj` it should not raise an `IndexError`

MNT: remove redundant imports

8495a1b

Both iterable and index_of are imported at the top of this import block

STY: minor pep8 white space

e48f7e2

FIX: non-indexed pd.Series into hist

9c37e3f

closes matplotlib#5557

TST: test hist with no 0 index pd.Series

f959cbe

DOC: fix typo / clarify docstring

bb7691b

tacaswell force-pushed the fix_pandas_indexing branch from f0a8a02 to bb7691b Compare December 29, 2015 00:02

tacaswell added 2 commits December 29, 2015 15:33

MNT: do not need to use six.next

1ba3a51

All versions of python we support have built in `next`

MNT: raise exception if generators passed in

bdaaf59

- moved next(iter(obj)) logic into a cbook function

tacaswell closed this Dec 29, 2015

tacaswell reopened this Dec 29, 2015

tacaswell added status: needs review and removed status: needs review labels Dec 29, 2015

WeatherGod added a commit that referenced this pull request Dec 30, 2015

Merge pull request #5556 from tacaswell/fix_pandas_indexing

aeafb64

FIX: pandas indexing error

WeatherGod merged commit aeafb64 into matplotlib:v1.5.x Dec 30, 2015

tacaswell removed the status: needs review label Dec 30, 2015

has2k1 mentioned this pull request Mar 12, 2016

Fix: Pandas indexing Error in collections #6148

Merged

tacaswell pushed a commit that referenced this pull request Mar 12, 2016

Fix: Pandas indexing Error in collections

3418f46

Specifically, when creating collection objects, `linewidths` and `antialiaseds` can be non zero indexed pandas series. Related to: PR #5556

tacaswell deleted the fix_pandas_indexing branch May 8, 2016 22:08

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jul 9, 2016

FIX: support np.flatiter is input again

03473e0

This was unintentionally broken by bdaaf59 which went in as part of PR matplotlib#5556 which was fixing support for pandas series.

tacaswell mentioned this pull request Jul 9, 2016

FIX: support np.flatiter is input again #6712

Merged

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jul 12, 2016

FIX: support np.flatiter as input again

8d03299

This was unintentionally broken by bdaaf59 which went in as part of PR matplotlib#5556 which was fixing support for pandas series.

Uh oh!

FIX: pandas indexing error #5556

FIX: pandas indexing error #5556

Uh oh!

Conversation

tacaswell commented Nov 24, 2015

Uh oh!

has2k1 Dec 12, 2015

Choose a reason for hiding this comment

Uh oh!

tacaswell Dec 13, 2015

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Dec 21, 2015

Uh oh!

tacaswell commented Dec 22, 2015

Uh oh!

WeatherGod Dec 28, 2015

Choose a reason for hiding this comment

Uh oh!

tacaswell Dec 28, 2015

Choose a reason for hiding this comment

Uh oh!

WeatherGod commented Dec 28, 2015

Uh oh!

tacaswell commented Dec 28, 2015

Uh oh!

WeatherGod commented Dec 28, 2015

Uh oh!

tacaswell commented Dec 28, 2015

Uh oh!

WeatherGod commented Dec 28, 2015

Uh oh!

tacaswell commented Dec 28, 2015

Uh oh!

WeatherGod commented Dec 28, 2015

Uh oh!

tacaswell commented Dec 29, 2015

Uh oh!

WeatherGod commented Dec 29, 2015

Uh oh!

tacaswell commented Dec 29, 2015

Uh oh!

WeatherGod commented Dec 29, 2015

Uh oh!

tacaswell commented Dec 29, 2015

Uh oh!

WeatherGod commented Dec 29, 2015

Uh oh!

tacaswell commented Dec 29, 2015

Uh oh!

tacaswell commented Dec 29, 2015

Uh oh!

tacaswell commented Dec 29, 2015

Uh oh!

WeatherGod commented Dec 29, 2015

Uh oh!

mdboom commented Dec 29, 2015

Uh oh!

WeatherGod commented Dec 29, 2015

Uh oh!

tacaswell commented Dec 29, 2015

Uh oh!

WeatherGod commented Dec 29, 2015

Uh oh!

WeatherGod commented Dec 30, 2015

Uh oh!

WeatherGod commented Dec 30, 2015

Uh oh!

Uh oh!