API/BUG: Fix Series ops inconsistencies #13894
Conversation
sinhrks
added
API Design
Numeric Operations
sinhrks
force-pushed
the
ops_series_compat
branch
from
ae6a4fd
to
76c76c8
Compare
Current coverage is 85.25% (diff: 100%)@@ master #13894 diff @@
==========================================
Files 139 139
Lines 50386 50394 +8
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 42959 42965 +6
- Misses 7427 7429 +2
Partials 0 0
|
| - ``Series`` comparison operators now raise ``ValueError`` when ``index`` are different. | ||
| - ``Series`` logical operators align both ``index``. | ||
|
|
||
| As a result, ``Series`` and ``DataFrame`` operators behave consistently as below: |
There was a problem hiding this comment.
add a comment on how this is more strict than previously. (maybe in a warning). IOW in cases where it previously worked, it will now error if the alignment is incorrect. This will now raise rather than silently pass.
sinhrks
force-pushed
the
ops_series_compat
branch
2 times, most recently
from
89515c9
to
3cf1f77
Compare
|
will have a look. |
| s1 & s2 | ||
|
|
||
| .. note:: | ||
| ``Series`` logical operators fill ``NaN`` result with ``False``. |
There was a problem hiding this comment.
This is inconsistent with how DataFrame behaves?
|
For dataframes, the operator (eg |
sinhrks
force-pushed
the
ops_series_compat
branch
from
3cf1f77
to
c9cde17
Compare
|
This seems OK to me. I'm sure this will cause some API breakage but it would be best to encourage folks to use the flex comparison methods if they want auto-alignment |
|
In light of Wes' comment, I think the whatsnew notice can be a bit more explicit in that regard. So say that if you did |
sinhrks
force-pushed
the
ops_series_compat
branch
2 times, most recently
from
b4980b5
to
b6a837b
Compare
| Until 0.18.1, comparing ``Series`` with the same length has been succeeded even if | ||
| these ``index`` are different (the result ignores ``index``). | ||
| As of 0.19.1, it raises ``ValueError`` to be more strict. | ||
|
|
There was a problem hiding this comment.
As I said in another comment, I would be more explicit here about how to deal with this when you used such code (so in case you wanted this behaviour, you now have to use s1.values == s2.values)
Maybe an example where you show all 4 cases (old behaviour, new default, new .values, new flex method) next to each other. The overview below is very structured, which is certainly good to keep! But I would also give one example for probably the case where most likely breakages will occur that compares all different behaviour together.
|
Modulo the doc comments, this is good to go for me! |
sinhrks
force-pushed
the
ops_series_compat
branch
from
b6a837b
to
fe322be
Compare
|
LGTM |
|
@sinhrks Thanks a lot! |
git diff upstream/master | flake8 --diffThis includes the fix only for #1134 and not for other inconsistencies (like #13637), as it needs further discussions.
Changes: