COMPAT: numpy master compat for groupby-filter
State: closed

xref https://github.com/numpy/numpy/pull/8591

I think we were relying on some sketchy behavior

https://travis-ci.org/pandas-dev/pandas/jobs/200459486

in ``pandas/core/groupby.py``
```
        for name, group in gen:
            object.__setattr__(group, 'name', name)
    
            res = func(group, *args, **kwargs)
    
            try:
                res = res.squeeze()
            except AttributeError:  # allow e.g., scalars and frames to pass
                pass
    
            # interpret the result of the filter
            if is_bool(res) or (is_scalar(res) and isnull(res)):
                if res and notnull(res):
                    indices.append(self._get_index(name))
            else:
                # non scalars aren't allowed
                raise TypeError("filter function returned a %s, "
                                "but expected a scalar bool" %
>                               type(res).__name__)
E               TypeError: filter function returned a ndarray, but expected a scalar bool
```

Labels:
name: Compat
description: pandas objects compatability with Numpy or Python functions

Comments:
cc @shoyer 
cc @charris
This looks like https://github.com/numpy/numpy/pull/8591, possibly?
CC @eric-wieser

Agreed that this looks like pandas was relying on sketchy behavior
I just find the numpy behavior of

``np.bool_(False)`` not being the *same* as ``np.array(False, dtype=bool)`` this is just very confusing / buggy IMHO.

But I understand this is a bigger issue w.r.t. scalar / 0-dim arrays.
So this means NumPy 1.13 would cause problems with older versions of Pandas? Perhaps we should issue a FutureWarning instead and wait a couple of releases. That particular fix was somewhere in between a bug fix and an official behavior change, I wasn't quite sure what to do with it.
@charris yes I suppose that this could cause a problem. Up to you about warning though. Its really only on filtering with groupby when using a specific type of function. 
I suppose the other option would be to make python scalars consistent rather than the current behavior.
```
In [7]: squeeze(1)
Out[7]: array(1)

In [8]: squeeze(int8(1))
Out[8]: 1
```
That might break less code.


that would be aok with me!
@eric-wieser @jreback  So the proposal would be to return the input unchanged unless it had a `squeeze` method and leave the previous behavior for scalars unchanged.
@charris: So you want to make the fix the other way, and have `np.squeeze(np.int32(1))` return `np.int32(1)`?

That's still better than the current behaviour, but I think it's more useful for np.squeeze to always return an array though,
using the reasoning that `np.squeeze` should be idempotent, and `np.squeeze(np.squeeze(x)) == np.squeze(x)` for all x/
 (this is true without that patch)

I'd be in favor of reverting the patched behaviour, and adding a `FutureWarning` that the change is pending
@eric-wieser Yeah, I can't quite figure the optimal behavior. We want to be able to pass array_likes that don't have a `squeeze` method (lists, tuples, etc.), and making exceptions for array_like scalars might be a bit odd. One way to do that would be to check the result of `asarray` and return the input if it is 0-D. Numpy scalars and array scalars would not show up there as they have the `squeeze` method and would preserve current behavior. Or, we could issue the `FutureWarning` and always return an array at some future date.

I'd actually like to raise the issue on the mailing list for discussion. I'm leaning towards preserving the current scalar behavior but others might differ.
@eric-wieser Oh, should I mail the mailing list, or would you like to do that?
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/15365?src=pr&el=h1) Report
> Merging [#15365](https://codecov.io/gh/pandas-dev/pandas/pull/15365?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/61deba5cfc43425e35c8fc61bcad1123c83a6a5a?src=pr&el=desc) will **decrease** coverage by `-0.01%`.


```diff
@@            Coverage Diff             @@
##           master   #15365      +/-   ##
==========================================
- Coverage   90.32%   90.32%   -0.01%     
==========================================
  Files         135      135              
  Lines       49439    49440       +1     
==========================================
- Hits        44658    44657       -1     
- Misses       4781     4783       +2
```


| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/15365?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/groupby.py](https://codecov.io/gh/pandas-dev/pandas/compare/61deba5cfc43425e35c8fc61bcad1123c83a6a5a...2250292516e5ad0c83f872bf5834b5fbb50e4858?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS5weQ==) | `95.04% <100%> (-0.1%)` | :x: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/15365?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/15365?src=pr&el=footer). Last update [61deba5...2250292](https://codecov.io/gh/pandas-dev/pandas/compare/61deba5cfc43425e35c8fc61bcad1123c83a6a5a...2250292516e5ad0c83f872bf5834b5fbb50e4858?el=footer&src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
yeah I am of the opinion that squeezing a scalar should not cause it to become an array. Ideally we dont' ever have 0-dim arrays, they are just confusing and we immediately unbox them anyhow.
@jreback

> Ideally we dont' ever have 0-dim arrays, they are just confusing and we immediately unbox them anyhow.

I disagree with this. Without 0-dim arrays, it's impossible to preserve array subclasses. If anything, I think it's the scalars that have no place. All the confusion that I've seen arise comes from using `np.isscalar(x)` rather than `np.ndim(x) == 0`

@charris : I'll let you email the mailing list
What is this ` res = res.squeeze()` line even trying to do? That's never going to return a scalar unless `res` was already a scalar:

```
>>> np.squeeze([1])
array(1)
```
@eric-wieser 

> I disagree with this. Without 0-dim arrays, it's impossible to preserve array subclasses.

maybe for numpy. but this is just plain confusing to pandas and higher stack. We do away with 0-dim anything.

> What is this res = res.squeeze() line even trying to do? That's never going to return a scalar unless
``Series`` and ``DataFrame`` both support this and rationally return a scalar if there is only a single element. numpy is not playing nice here.

``np.squeeze(scalar)`` -> ``scalar`` is the only rationale thing to do. 
> numpy is not playing nice here.

Numpy has picked a different contract for its `squeeze` method to pandas, but unfortunately they share the same name, which causes duck-typing to fail.

I think there are bigger disagreements about what `squeeze` means here than just `squeeze(scalar) -> scalar`:

```python
>>> pd.DataFrame([1]).squeeze()
1
>>> np.array([1]).squeeze()
np.array(1)
```

Numpy explicitly chooses to behave in this way for 1D arrays, by [guaranteeing that](https://docs.scipy.org/doc/numpy-1.10.0/reference/generated/numpy.squeeze.html):

> [the result] is always [the input] itself or a view into  [the input].

A scalar result can never be a view into an array.

---

Given that the behaviour of `squeeze` is different in many ways already, I don't think there's an argument beyond backwards-compatibility for making it match in this one way

Although I guess numpy breaks its own contract here by duck-typing as well:
```python
>>> p = pd.DataFrame([1])
>>> np.squeeze(p)
1  #oops - that's not a view!
```
@eric-wieser 

yeah I don't *need* to use it, just *was*. I don't care what numpy does as long as its consistent :>
going to hide it anyhow. But ``np.squeeze`` on a scalar returning an ndarray is really wrong IMHO.
@eric-wieser I wouldn't take the documentation too seriously, it wouldn't be the first time the documentation was incorrect. I expect the original behavior with numpy scalars was deliberately chosen, so at this point we are really trying to decide what the contract should be.
Actually, I think I've now switched to agree, since `squeeze(scalar) -> array` would violate the `input or view` rule, and give the illusion that modifying the output would affect the input.
@eric-wieser I reverted the change in https://github.com/numpy/numpy/pull/8610.
thanks @charris