Skip to content

BUG: Matched SearchedSorted Signature with NumPy's #12413

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

BUG: Matched SearchedSorted Signature with NumPy's #12413

wants to merge 1 commit into from

Conversation

@gfyoung
Copy link
Member

@gfyoung gfyoung commented Feb 22, 2016

Title is self-explanatory. Closes #12238.

@gfyoung
Copy link
Member Author

gfyoung commented Feb 22, 2016

Where should I put tests for this compatibility issue? It would be great if I could just write one test for all of the classes that have a searchsorted method.

@jreback
Copy link
Contributor

jreback commented Feb 22, 2016

test_base will exercise this

@gfyoung gfyoung force-pushed the searchsorted_compat branch from d22a921 to 6c70e12 Compare February 22, 2016 09:25
@gfyoung
Copy link
Member Author

gfyoung commented Feb 22, 2016

Added test, and the test passes on Travis. Should be good to merge if there is nothing else.

@gfyoung gfyoung closed this Feb 22, 2016
@gfyoung gfyoung reopened this Feb 22, 2016
@gfyoung
Copy link
Member Author

gfyoung commented Feb 22, 2016

I need to get a new mouse. In any case, I did let the tests run with my new test, and Travis gave the green light.

jreback
jreback reviewed Feb 22, 2016
View reviewed changes
pandas/tests/test_base.py Outdated
Copy link
Contributor

@jreback jreback Feb 22, 2016

Choose a reason for hiding this comment

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

no, see how we do for example .value_counts where an object is constructed and looped thru. pls follow the same pattern

@jreback jreback added Indexing Related to indexing on series/frames, not to indexes themselves Compat pandas objects compatability with Numpy or Python functions labels Feb 22, 2016
@gfyoung gfyoung force-pushed the searchsorted_compat branch 4 times, most recently from d3792d2 to 6cb665f Compare February 23, 2016 08:33
@gfyoung
Copy link
Member Author

gfyoung commented Feb 23, 2016

Tests are finally passing! Should be good to merge if there is nothing else.

jreback
jreback reviewed Feb 23, 2016
View reviewed changes
doc/source/whatsnew/v0.18.0.txt Outdated
Copy link
Contributor

@jreback jreback Feb 23, 2016

Choose a reason for hiding this comment

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

you don't need to say all this, just say searchsorted now accepts sorter arg for compat with numpy.

Copy link
Member Author

@gfyoung gfyoung Feb 23, 2016

Choose a reason for hiding this comment

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

Done.

@jreback
Copy link
Contributor

jreback commented Feb 23, 2016

you can put in 0.18.0

@jreback jreback added this to the 0.18.0 milestone Feb 23, 2016
@gfyoung gfyoung force-pushed the searchsorted_compat branch 4 times, most recently from a28afb0 to 62ca892 Compare February 23, 2016 16:57
@gfyoung
Copy link
Member Author

gfyoung commented Feb 23, 2016

You accidentally cancelled all of my builds for this PR.

@jreback
Copy link
Contributor

jreback commented Feb 23, 2016

oh sorry, push again :)

@gfyoung gfyoung force-pushed the searchsorted_compat branch from 62ca892 to e147977 Compare February 23, 2016 17:29
@gfyoung
Copy link
Member Author

gfyoung commented Feb 23, 2016

FYI: I believe you can restart a build, but no worries here since I just rebased onto master as my new "commit".

@gfyoung
Copy link
Member Author

gfyoung commented Mar 16, 2016

@jreback : Also, regarding that side typo I made with Categorical initially, I don't think that was ever an issue because the line before it made sure that the side parameter was enforced.

@gfyoung gfyoung force-pushed the searchsorted_compat branch from 887d4dc to f350442 Compare March 16, 2016 16:30
@jreback
Copy link
Contributor

jreback commented Mar 16, 2016

@gfyoung ok then

jreback
jreback reviewed Mar 16, 2016
View reviewed changes
pandas/core/base.py Outdated
Copy link
Contributor

@jreback jreback Mar 16, 2016

Choose a reason for hiding this comment

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

I know this is annoying, but how about we rename v -> value. More readable args.

Copy link
Member Author

@gfyoung gfyoung Mar 16, 2016

Choose a reason for hiding this comment

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

In a previous comment we changed it to v so that it matches the signature of numpy, or maybe I misunderstood you then?

Copy link
Contributor

@jreback jreback Mar 16, 2016

Choose a reason for hiding this comment

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

no I think it was always like this. But we generally name things as value in pandas (single character is too like q/kdb .... )

Copy link
Member

@jorisvandenbossche jorisvandenbossche Mar 16, 2016

Choose a reason for hiding this comment

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

Let's maybe just keep it at v (as long as we do it better in our own methods!), you never know that someone used it as a keyword argument

Copy link
Member Author

@gfyoung gfyoung Mar 16, 2016

Choose a reason for hiding this comment

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

@jorisvandenbossche : v is not used as a keyword in numpy, and from a semantic point of view, I see no reason why it would be.

Copy link
Member Author

@gfyoung gfyoung Mar 16, 2016

Choose a reason for hiding this comment

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

Yikes, this is news to me. I guess I never encountered that before. Well, at least I know now and am slightly less of a Python noob because of that. Thanks for clarifying @kawochen!

Copy link
Contributor

@jreback jreback Mar 17, 2016

Choose a reason for hiding this comment

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

ok, just about to merge, then I saw you changed some of the v -> value. (you missed the datetimelikes).

So originally we had datetimelikes used key (and I had you change it to v). While Series used v.

So we have to break something.

Let's just keep the rename to value (and fix the datetimelikes). Move the whatsnew note to API Changes (and note that the argument name changed). I seriously doubt anyone is actually passing as a kw arg (the first arg).

Copy link
Member Author

@gfyoung gfyoung Mar 17, 2016

Choose a reason for hiding this comment

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

Okay, so rename to value and make a note in the API changes? Also, what do you mean by "datetimelikes"?

Copy link
Contributor

@jreback jreback Mar 17, 2016

Choose a reason for hiding this comment

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

datetimes, timedelta, period

Copy link
Member Author

@gfyoung gfyoung Mar 17, 2016

Choose a reason for hiding this comment

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

Got it. Done.

@gfyoung gfyoung force-pushed the searchsorted_compat branch 2 times, most recently from aa140f7 to 7e37570 Compare March 16, 2016 17:28
@codecov-io
Copy link

codecov-io commented Mar 16, 2016

Powered by Codecov. Updated on successful CI builds.

@gfyoung gfyoung force-pushed the searchsorted_compat branch 2 times, most recently from 5b33a6c to 3a0de8b Compare March 17, 2016 04:08
@gfyoung
Copy link
Member Author

gfyoung commented Mar 17, 2016

@jreback : Changes have been made, and Travis approves. Should be good to merge then.

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Mar 17, 2016

Sorry, but I want to say it again. Changing a keyword's name is an API change that can break people's code, and since we want to change it because "we don't like the short name", it is certainly not an unavoidable API change, so IMO should not go into a minor release.

@gfyoung
Copy link
Member Author

gfyoung commented Mar 17, 2016

@jorisvandenbossche : I find myself caught in a tug-of-war between you and @jreback on this issue, not to mention the push to land this is very high. It would be great if some consensus could be found on this.

jreback
jreback reviewed Mar 17, 2016
View reviewed changes
doc/source/whatsnew/v0.18.1.txt Outdated
Copy link
Contributor

@jreback jreback Mar 17, 2016

Choose a reason for hiding this comment

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

pls just make the whatsnew simpler. just say the signature changed the names of the arguments and put in a pointer to the signature.

Copy link
Member Author

@gfyoung gfyoung Mar 17, 2016

Choose a reason for hiding this comment

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

Done. I put [ci skip] in my commit because this is only a doc change. So it should be good to merge if there is nothing else.

@gfyoung gfyoung force-pushed the searchsorted_compat branch 2 times, most recently from a30e10f to d7690c2 Compare March 17, 2016 15:48
@jreback
Copy link
Contributor

jreback commented Mar 17, 2016

@jorisvandenbossche well, we already have that issue as key is the name for the datetimelikes and v for Series. I guess its ok to move this part of the change to 0.19.0 (the actual name changes). Though really don't think this is a big deal to change this now as its the first argument.

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Mar 17, 2016

But I think it is also not a big deal to just leave it for 0.19.0, just in case to be safe. But I agree the discrepancy with Index key is indeed also not nice.

@gfyoung would you like to split the PR in two? one for the actual fix to searchsorted and one for the name change?

@jreback
Copy link
Contributor

jreback commented Mar 17, 2016

ok, let's move the name change to 0.19.0. (you can just create an issue I think and I'll mark it). otherwise ok then. (back out both names changes, IOW reverse the value -> key for datetimelikes)

@gfyoung gfyoung mentioned this pull request Mar 17, 2016
Closed
@gfyoung gfyoung force-pushed the searchsorted_compat branch from d7690c2 to 1e6df5e Compare March 17, 2016 20:30
@gfyoung
Copy link
Member Author

gfyoung commented Mar 17, 2016

@jreback : Travis has finally given the green light on this one as well. Should be good to merge if there is nothing else to add.

@jreback jreback closed this in 28327ce Mar 17, 2016
@jreback
Copy link
Contributor

jreback commented Mar 17, 2016

whoosh! thanks for the patience

@gfyoung
Copy link
Member Author

gfyoung commented Mar 17, 2016

@jreback: No worries. Onto fixing the rest of fromnumeric.py!

@gfyoung gfyoung deleted the searchsorted_compat branch March 17, 2016 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Compat pandas objects compatability with Numpy or Python functions Indexing Related to indexing on series/frames, not to indexes themselves

Projects

None yet

Milestone

0.18.1

Development

Successfully merging this pull request may close these issues.

6 participants

@gfyoung @jreback @shoyer @kawochen @jorisvandenbossche @codecov-io