BUG: Matched SearchedSorted Signature with NumPy's
State: closed

Title is self-explanatory.  Closes #12238.


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

Comments:
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.

test_base will exercise this

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

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.

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

you can put in 0.18.0

You accidentally cancelled all of my builds for this PR.

oh sorry, push again :)

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

right, but just as easy to:

`git commit --all --amend -C HEAD` to generate a new commit as well.

Ah, good to know!

We should raise `NotImplementedError` if sorter is not None. It's not a good idea to insert arguments without using them.

@shoyer : I'm a little wary of doing that because it breaks compatibility with `numpy`.  That is the whole point of this PR.  It's not pretty, but unfortunately, it is necessary.

I agree with @shoyer.  I don't think it would break compatibility since it's just validation.

NumPy needs to be able to pass the `sorter` argument on to pandas so the `np.searchsorted` wrapper works. But the default sorter is `None`, which is the same default we'll use here for the pandas method. Given that we don't support other values for `sorter`, we should raise an error if that happens.

Fair enough.  `NotImplementedError` added.

Tests are passing once more.  Should be good to merge if there is nothing else.

I am testing out hiding the 'sorter' argument.  This is WIP.  There are some instances in the code base where the `sorter` argument is actually used (e.g. `tests/test_categorical.py` and `core/series.py`).  Should I just remove them?  And if so, is this more of an API change than a bug fix?

We don't want to remove existing cases where `sorter` works. If anything, I would make `sorter` work in more places (instead of requiring that the argument always be `None`).

@shoyer : I think I'll leave you to discuss that point with @jreback in this PR.  He seems to be of the opposite camp that the `sorter` argument doesn't add much for the `pandas` implementation.  TBH, I don't know how often it is used in either library, so I'm not sure which way is best.

@jreback : Did the refactoring, and tests are passing.

minor naming change - otherwise lgtm

As I mentioned inline above, I think this needs to raise an exception for an invalid `sorter` argument. Existing precedent for buggy APIs is not a good excuse for making new buggy designed APIs. @jreback, your thoughts?

@jreback : Renamed the functions, and Travis remains happy.

will give another look later, but my general philosophy is in-line with @shoyer maybe it didn't come across.

We don't want to advertise `sorter` when used with `s.searchsorted(...)` IOW a pandas method. But don't want to disallow passing with `np.searchsorted(.....)`. So I suppose we can use completely ignore the `sorted` argument (if passed), as its not clear when its passed for compat reasons (e.g. from INSIDE `np.searchsorted`), or the user passing it in. but not sure it really matters.

@jreback : Sure thing.  I think the part that seems conflicting between you and @shoyer is the handling of `numpy` arguments when they are not their default values.  While you seemed to have preferred to just ignore it (that was the impression I got when I wrote the `_validate_kwargs` method in a previous PR), @shoyer is advocating throwing an `Exception` instead.

While I don't know how `searchsorted` will work in the future, but FYI, the way it works when you call `np.searchsorted` is that it checks whether the array-like object being passed in has its own `searchsorted` method.  If it does, it calls that method using the signature `(val, side=`left`, sorter=None)`.  If it does not have such a method, the object is wrapped in a `numpy` array and that array's `searchsorted` method is called instead.

I know we have beat this to death! looks pretty good. Just a comment about why special casing the check for `sorter`, further why are we not accepting `**kwargs` (in addition to `*arg`) for `.searchsorted()`? (e.g. make sure it is empty).

Updated PR with your comments regarding the documentation.  Added `[ci skip]` since it is code-wise no different from build <a href="https://travis-ci.org/pydata/pandas/builds/116031498">#18292</a>, which I am still waiting to pass, though it should most likely pass since it passed locally.

ok, so to lay out a policy here which is compat with several goals.
- we don't really need or want the `sorter` argument, but `numpy` requires it (as a positional). So am forced to accept it and document it as an argument that we just pass thru. So `Series` is good now, @gfyoung let's keep the modification where you put it (IOW have `Categorical/Index`) support this so its all in 1 place. Remove the `*args, **kwargs` checking. You can put back minimal tests for passing this thru (numpy will do all error checking). Make a note in the doc-string that this is just a pass thru argument.

The reasoning here is that we have a positional argument that we are forced to accept for compat reasons. We cannot easily ignore this w/o jumping thru a bunch of hoops. We are forced to have this in the doc-string.
- other PR (about `np.round`) and stat functions. We _need_ to accept `out`, but explicity don't do anything or want this argument. Here let's check if its `None` (its in `**kwargs` already), if not raise a `TypeError` (we do this already). Further goal is since we are accepting `**kwargs` need to make sure that we dont take addtl args that are not in the doc-string.

The reasoning here is we have a kwargs argument that we don't want (and don't want in the doc-string), and can safely make sure its `None`

Bottom line is that we don't want to have arbitrary / odd argument in the doc-strings (and signatures) just for compat, unless we have to jump thru hoops to get rid of them. Having arguments which don't do anything is just bad practice in general and makes things even more confusing. 

In particular I am (I guess was) making a case that `sorter` is a really numpy specific arg that in pandas space is completely useless and misleading (we NEVER need this, we ALREADY have a concept of an index and values; numpy NEEDS this because it doesn't).

I think it is worthwhile to hide completely numpy specific useless things from the user. We do this for many many other cases. So philosophically I don't see any reason to digress here. The reason I am yielding is that there _is_ a slight usecase to support `np.searchsorted(...)` and the fact that numpy passes these positionally BACK to use (with `_wrap_it`), is just plain confusing (and @gfyoung has already opened a report on this, but this can't affect our decision here because of back-compat with older numpies).

We have in the past few years been more and more hiding numpy from end-user for normal cases. I think this is just going to increase to where you won't be able to TELL if numpy is there or not, it just won't matter at all. Therefor anything that can reduce friction with the end-user and reduce confusion is a good thing, even at the expense of some work-arounds.

Alright, let me see if I understand this correctly:

For compatibility reasons, we are going to add `sorter` as a **keyword** argument for all relevant data structures (i.e. `Index`, `Category`, `DataFrame`, etc...).  However, for the majority of these data structures (excluding `Series`), this will serve as a **pass thru** argument that will be validated as per @shoyer.  For `Series`, I am personally in favor of deprecating `sorter` because it's going to be confusing for the user if we support it for one data structure but not for the others.

In this case, I'll remove the `*args` and `**kwargs` checking and move that into my `np.round` PR and remove its dependency on this one.  Also, @jreback , unfortunately, it will have to be an `*args` thing because `numpy` passes it in internally as a positional argument despite accepting as a keyword!