Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

FIX: pandas indexing error#5556

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
WeatherGod merged 10 commits intomatplotlib:v1.5.xfromtacaswell:fix_pandas_indexing
Dec 30, 2015

Conversation

tacaswell
Copy link
Member

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

@tacaswelltacaswell added this to theCritical bugfix release (1.5.1) milestoneNov 24, 2015
@@ -1561,7 +1561,7 @@ def default_units(x, axis):
x = x.ravel()

try:
x =x[0]
x =next(iter(x))
except (TypeError, IndexError):
Copy link
Contributor

Choose a reason for hiding this comment

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

line1565 should be modified to

except (TypeError,StopIteration):

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Thanks for catching that!

@tacaswell
Copy link
MemberAuthor

rebased.

@tacaswell
Copy link
MemberAuthor

Also addresses#5557

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

Choose a reason for hiding this comment

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

"is" --> "in"?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I swear I know english...

@WeatherGod
Copy link
Member

besides the nitpicking, this looks good to me

@tacaswell
Copy link
MemberAuthor

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

@WeatherGod
Copy link
Member

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

@tacaswell
Copy link
MemberAuthor

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

@WeatherGod
Copy link
Member

?

@tacaswell
Copy link
MemberAuthor

The comment below the commented line says that we are handlingnormed 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
Copy link
Member

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 positionalindexing.  This method will get the first element of any iterable.It will advance a generater, but they did not work previously anyway.Closesmatplotlib#5550
using `next(iter(obj))` can raise `TypeError` if obj is not iterableand `StopIteration` if len(obj) == 0.  As we are no longer explictilyindexing into `obj` it should not raise an `IndexError`
Both iterable and index_of are imported at the top of this import block
@tacaswell
Copy link
MemberAuthor

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

@WeatherGod
Copy link
Member

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
Copy link
MemberAuthor

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 notbreak any existing user code.

@WeatherGod
Copy link
Member

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

@tacaswell
Copy link
MemberAuthor

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

@WeatherGod
Copy link
Member

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
Copy link
MemberAuthor

fair enough

All versions of python we support have built in `next`
 - moved next(iter(obj)) logic into a cbook function
@tacaswell
Copy link
MemberAuthor

Digging into this a bit (and trying to sort out whyplot does not have this problem) it turns out thatplot goes through an entirely different code path by usingAxis.update_units which delegates tounits.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
Copy link
MemberAuthor

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

@WeatherGod
Copy link
Member

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

@mdboom
Copy link
Member

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
Copy link
Member

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

@tacaswell
Copy link
MemberAuthor

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

@WeatherGod
Copy link
Member

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

@WeatherGod
Copy link
Member

Looks like you cleared out the rabbits!

WeatherGod added a commit that referenced this pull requestDec 30, 2015
@WeatherGodWeatherGod merged commitaeafb64 intomatplotlib:v1.5.xDec 30, 2015
@WeatherGod
Copy link
Member

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.

tacaswell pushed a commit that referenced this pull requestMar 12, 2016
Specifically, when creating collection objects,`linewidths` and `antialiaseds` can be non zero indexedpandas series.Related to: PR#5556
@tacaswelltacaswell deleted the fix_pandas_indexing branchMay 8, 2016 22:08
tacaswell added a commit to tacaswell/matplotlib that referenced this pull requestJul 9, 2016
This was unintentionally broken bybdaaf59 which went in as part ofPRmatplotlib#5556 which was fixing support for pandas series.
tacaswell added a commit to tacaswell/matplotlib that referenced this pull requestJul 12, 2016
This was unintentionally broken bybdaaf59 which went in as part ofPRmatplotlib#5556 which was fixing support for pandas series.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.5.1
Development

Successfully merging this pull request may close these issues.

4 participants
@tacaswell@WeatherGod@mdboom@has2k1

[8]ページ先頭

©2009-2025 Movatter.jp