CLN: remove unnecessary check `needs_i8_conversion` if Index subclass does not support `any` or `all` #58006

natmokval · 2024-03-25T22:56:32Z

removed unnecessary check needs_i8_conversion if Index subclass does not support any or all.

jbrockmendel · 2024-03-26T02:07:24Z

pandas/core/indexes/base.py

-        ):
-            # This call will raise
-            make_invalid_op(opname)(self)
+        if isinstance(self, ABCMultiIndex) or self.dtype.kind != "m":


is the != "m" bit still needed?

Thanks for the comment. I think we don't need != "m" here. This check worked only together with the check needs_i8_conversion(self.dtype).

I removed != "m" and corrected tests.

I have a question: shouldn't we raise for DatetimeIndex? So far we show FutureWarning:
"'any' with datetime64 dtypes is deprecated and will raise in a future version. Use (obj != pd.Timestamp(0)).any() instead."

there is a deprecation in nanany and nanall that i think is intended to be enforced before this needs_i8_conversion check is removed

thanks, it was my thought too.
Should I open new PR to enforce the deprecation in nanany and nanall, or can I do it in this PR?

I enforced deprecation any/all with datetime64 in #58029

pandas/tests/indexes/numeric/test_numeric.py

pandas/core/indexes/base.py

Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>

WillAyd · 2024-03-28T03:08:04Z

pandas/tests/indexes/test_old_base.py

@@ -228,6 +233,8 @@ def test_logical_compat(self, simple_index):
                    r"'IntervalArray' with dtype interval\[.*\] does "
                    "not support reduction '(any|all)'"
                )
+            if isinstance(idx, PeriodIndex):


I think you can combine this branch with the IntervalArray branch directly preceding it and reduce the msg check to does not support reduction '(any|all)'

It is generally a good idea to try and reduce the amount of branching in tests. Having a bunch of if...else statements makes it unnecessarily harder to manage consistent behavior expectations

…//github.com/natmokval/pandas into remove-unnecessary-check-needs_i8_conversion

natmokval · 2024-03-29T22:59:02Z

thank you @WillAyd , I combined these two branches and reduced the message checks as you suggested.

I think if we change the error message in def _reduce

pandas/pandas/core/arrays/base.py

Lines 1898 to 1901 in 4241ba5

    
           raise TypeError( 
        
               f"'{type(self).__name__}' with dtype {self.dtype} " 
        
               f"does not support reduction '{name}'" 
        
           )

to

f"'{type(self).__name__}' with dtype {self.dtype} "
f"does not support operation: '{name}'"

we can reduce the amount of branching in tests. Think that's a good idea?

WillAyd · 2024-04-01T11:53:10Z

pandas/tests/indexes/test_old_base.py

@@ -223,11 +223,10 @@ def test_logical_compat(self, simple_index):
            assert idx.any() == idx.to_series().any()
        else:
            msg = "cannot perform (any|all)"


I think you can delete L225-L228? i.e. the only thing needed here to pass tests is msg = "does not support reduction '(any|all)'"?

thanks, I deleted the branch with msg = "datetime64 type does not support operation: '(any|all)'", ci- green.

Oh OK I see the issue now. So you are branching because DatetimeIndex says does not support operation: '(any|all)' but the other cases say 'does not support reduction '(any|all)'

I don't want to go down a rabbit hole but having a message that differs between operation: and reduction doesn't seem that important. How hard is it to align them so you can just assign msg = "does not support operation: '(any|all)' and have the tests pass?

WillAyd · 2024-04-02T02:38:24Z

pandas/tests/indexes/test_old_base.py

@@ -213,7 +213,7 @@ def test_numeric_compat(self, simple_index):
            1 // idx

    def test_logical_compat(self, simple_index):
-        if simple_index.dtype in (object, "string"):
+        if simple_index.dtype in (object, "string", "datetime64[ns]"):


Is the datetime skip required here? I am not overly familiar with this part of the code base but judging by the deleted comment I didn't think this needed to change

I think we shouldn't skip datetime64 here. We need the separate branch for datetime64 because we get different error message: "datetime64 type does not support operation: '(any|all)'"

If we replace the error message

pandas/pandas/core/nanops.py

Lines 521 to 523 in 6f39c4f

if values.dtype.kind == "M":

# GH#34479

raise TypeError("datetime64 type does not support operation: 'any'")

with the msg "does not support reduction '(any|all)'" we can combine these two checks. Can I do it in this PR or a new one?

I suggest to align the error messages and assign msg = “.* does not support operation .*“ . I replaced reduction with operation and operation: with operation to unify error messages. Tests passed.

WillAyd

Thanks for aligning those - looks pretty good one minor thing

WillAyd · 2024-04-04T11:37:13Z

pandas/tests/indexes/test_old_base.py

@@ -223,11 +223,7 @@ def test_logical_compat(self, simple_index):
            assert idx.any() == idx.to_series().any()
        else:
            msg = "cannot perform (any|all)"


L225 looks like it can be safely deleted

WillAyd

lgtm when green

mroeschke · 2024-04-04T16:47:07Z

Thanks @natmokval

… does not support `any` or `all` (pandas-dev#58006) * cln-remove-unnecessary-check-needs_i8_conversion * fix tests * remove the check self.dtype.kind, fix tests * Update pandas/core/indexes/base.py Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com> * combine two branches in test_logical_compat * correct test_logical_compat * correct test_logical_compat * correct test_logical_compat * roll back the check for datetime64 in test_logical_compat * replace reduction with operation to unify err msg * delete unused line --------- Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>

cln-remove-unnecessary-check-needs_i8_conversion

c3cf9a1

natmokval added Index Related to the Index class or subclasses Reduction Operations sum, mean, min, max, etc. labels Mar 25, 2024

fix tests

54983c5

jbrockmendel reviewed Mar 26, 2024

View reviewed changes

pandas/tests/indexes/numeric/test_numeric.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Mar 26, 2024

View reviewed changes

pandas/tests/indexes/numeric/test_numeric.py Outdated Show resolved Hide resolved

remove the check self.dtype.kind, fix tests

3b2242f

natmokval marked this pull request as ready for review March 26, 2024 15:29

mroeschke reviewed Mar 26, 2024

View reviewed changes

pandas/core/indexes/base.py Outdated Show resolved Hide resolved

Update pandas/core/indexes/base.py

c253533

Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>

natmokval mentioned this pull request Mar 27, 2024

CLN: enforce any/all deprecation with datetime64 #58029

Merged

WillAyd requested changes Mar 28, 2024

View reviewed changes

natmokval added 6 commits March 29, 2024 19:55

Merge branch 'main' into remove-unnecessary-check-needs_i8_conversion

923aa2f

combine two branches in test_logical_compat

0b71ac1

Merge branch 'main' into remove-unnecessary-check-needs_i8_conversion

01ce724

correct test_logical_compat

add58c0

Merge branch 'remove-unnecessary-check-needs_i8_conversion' of https:…

be3ab97

…//github.com/natmokval/pandas into remove-unnecessary-check-needs_i8_conversion

correct test_logical_compat

18fab28

WillAyd reviewed Apr 1, 2024

View reviewed changes

correct test_logical_compat

c887a85

WillAyd reviewed Apr 2, 2024

View reviewed changes

natmokval added 2 commits April 2, 2024 18:26

roll back the check for datetime64 in test_logical_compat

bac52c5

replace reduction with operation to unify err msg

6efebb8

WillAyd requested changes Apr 4, 2024

View reviewed changes

delete unused line

4ff9cea

WillAyd approved these changes Apr 4, 2024

View reviewed changes

mroeschke approved these changes Apr 4, 2024

View reviewed changes

mroeschke added this to the 3.0 milestone Apr 4, 2024

mroeschke merged commit f4232e7 into pandas-dev:main Apr 4, 2024
46 checks passed

WillAyd added a commit to WillAyd/pandas that referenced this pull request Aug 22, 2024

Pick required fix from f4232e7 pandas-dev#58006

def32f5

WillAyd added a commit to WillAyd/pandas that referenced this pull request Aug 22, 2024

Pick required fix from f4232e7 pandas-dev#58006

94e52fd

WillAyd added a commit to WillAyd/pandas that referenced this pull request Aug 22, 2024

Pick required fix from f4232e7 pandas-dev#58006

2d5d9e4

WillAyd added a commit to WillAyd/pandas that referenced this pull request Aug 22, 2024

Pick required fix from f4232e7 pandas-dev#58006

1327d64

WillAyd added a commit to WillAyd/pandas that referenced this pull request Aug 22, 2024

Pick required fix from f4232e7 pandas-dev#58006

8dc27bc

WillAyd added a commit to WillAyd/pandas that referenced this pull request Aug 27, 2024

Pick required fix from f4232e7 pandas-dev#58006

c1f17f4

lithomas1 pushed a commit to lithomas1/pandas that referenced this pull request Sep 8, 2024

Pick required fix from f4232e7 pandas-dev#58006

db23861

WillAyd added a commit to WillAyd/pandas that referenced this pull request Sep 20, 2024

Pick required fix from f4232e7 pandas-dev#58006

88faf61

jorisvandenbossche pushed a commit to WillAyd/pandas that referenced this pull request Oct 2, 2024

Pick required fix from f4232e7 pandas-dev#58006

3001964

jorisvandenbossche pushed a commit to WillAyd/pandas that referenced this pull request Oct 2, 2024

Pick required fix from f4232e7 pandas-dev#58006

41e94c3

jorisvandenbossche pushed a commit to WillAyd/pandas that referenced this pull request Oct 3, 2024

Pick required fix from f4232e7 pandas-dev#58006

85ab066

jorisvandenbossche pushed a commit to WillAyd/pandas that referenced this pull request Oct 7, 2024

Pick required fix from f4232e7 pandas-dev#58006

172af49

jorisvandenbossche pushed a commit that referenced this pull request Oct 9, 2024

Pick required fix from f4232e7 #58006

41bd34f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN: remove unnecessary check `needs_i8_conversion` if Index subclass does not support `any` or `all` #58006

CLN: remove unnecessary check `needs_i8_conversion` if Index subclass does not support `any` or `all` #58006

natmokval commented Mar 25, 2024 •

edited

Loading

jbrockmendel Mar 26, 2024

natmokval Mar 26, 2024

jbrockmendel Mar 26, 2024

natmokval Mar 26, 2024

natmokval Mar 27, 2024

WillAyd Mar 28, 2024

natmokval commented Mar 29, 2024 •

edited

Loading

WillAyd Apr 1, 2024

natmokval Apr 1, 2024

WillAyd Apr 2, 2024

WillAyd Apr 2, 2024

natmokval Apr 2, 2024 •

edited

Loading

natmokval Apr 4, 2024

WillAyd left a comment

WillAyd Apr 4, 2024

WillAyd left a comment

mroeschke commented Apr 4, 2024

	if values.dtype.kind == "M":
	# GH#34479
	raise TypeError("datetime64 type does not support operation: 'any'")

CLN: remove unnecessary check needs_i8_conversion if Index subclass does not support any or all #58006

CLN: remove unnecessary check needs_i8_conversion if Index subclass does not support any or all #58006

Conversation

natmokval commented Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natmokval commented Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natmokval Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd left a comment

Choose a reason for hiding this comment

mroeschke commented Apr 4, 2024

CLN: remove unnecessary check `needs_i8_conversion` if Index subclass does not support `any` or `all` #58006

CLN: remove unnecessary check `needs_i8_conversion` if Index subclass does not support `any` or `all` #58006

natmokval commented Mar 25, 2024 •

edited

Loading

natmokval commented Mar 29, 2024 •

edited

Loading

natmokval Apr 2, 2024 •

edited

Loading