GH 9016: Bitwise operation weirdness

[tvyomkesh]

Series now supports bitwise op for integral types.

I have made the changes in wrapper() itself instead of na_op() because it looked to me like wrapper is controlling the input and output fill and dtype. Once that is taken care of, na_op() seems to be doing the right thing by itself and so I did not have to change anything to get back the expected behavior. Happy to change if things need to be altered toward better design etc.

[tvyomkesh]

This PR intends to resolve #9016

[shoyer]

the other.reindex_like(self) bit is repeated twice here -- can you consolidate it?

[jreback]

can u move the tests near the other boolean tests (right below) in test_series

also need some test for ops with NaN and series containing nan and ops with other dtypes
(with other dtypes should prob raise TypeError)

[tvyomkesh]

@shoyer, @jreback thanks, I am working on the suggested changes. In the meantime I noticed this. (using master without this patch).

In [4]: Series([0,1,2,3]) & Series([True, True, True, True])
Out[4]:
0    False
1     True
2    False
3     True
dtype: bool

I was expecting output to be Series([False, True, True, True]) as RHS has dtype bool.
Figured Numpy converts bool to 0/1 and executes bitwise op. For e.g.,

In [5]: import numpy as np

In [6]: np.array([0,1,2,3]) & np.array([True, True, True, True])
Out[6]: array([0, 1, 0, 1])

In [7]: np.array([0,1,2,3]) | np.array([True, True, True, True])
Out[7]: array([1, 1, 3, 3])

In the Series example, should the behavior be preserved? In that case should the output dtype be bool or int64? Otherwise it looks confusing.

[tvyomkesh]

Looks like if changed, test_logical_with_nas() in test_frame.py would break.

[tvyomkesh]

made the following changes:

• Code refactored to remove some duplication
• Moved test function near other comparison tests
• Added more tests with NaN and other dtypes
• Fixed code so it works with list,tuple and np.ndarray types correctly.

[jreback]

add a mini-example here

[jreback]

@tvyomkesh this seems like a lot of branching for just slightly different dtype handling. see what you can do about that.

[tvyomkesh]

@jreback thanks for pointing to core.common.is_interger_dtype(). Made the changes. Also cut down on branching a bit. Updated release note with a small example.

I still think this is confusing.

In [3]: pd.Series([2]) & pd.Series([True])
Out[3]:
0    False
dtype: bool

Either both the inputs should be converted to dtype boolean in which case output value should be True or if both the inputs are converted to dtype int64 implictly and bitwise & is carried out per current behavior, then the output dtype should remain int64 and not be coerced to bool. The current patch can be modified to fix this. Thoughts?

[tvyomkesh]

PR review changes are done. Good to merge?

[jreback]

you can collapse a lot of this if you define functions like this:

fill_bool = lambda x: x.fillna(False).astype(bool)
fill_int = lambda x: x.fillna(0)

filler = fill_int if is_self_int_dtype and is_other_int_dtype else fill_bool

other = filler(other)
res = filler(self._constructor(.......))

[tvyomkesh]

done

[tvyomkesh]

@jreback thanks. I have made the changes.

[jreback]

indent 2 spaces here (the code under the directive), otherwise won't render correctly

[tvyomkesh]

oh didn't notice this one. thanks.

[tvyomkesh]

@jreback and @shoyer I have pushed the changes. Thanks.

[jreback]

ok move this to the api section snd show s code block of the existing behavior (and then put this after)

[tvyomkesh]

done.

[jreback]

thanks @tvyomkesh