Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLN: enforce any/all deprecation with datetime64 #58029

Merged
merged 7 commits into from
Mar 28, 2024

Conversation

natmokval
Copy link
Contributor

@natmokval natmokval commented Mar 27, 2024

xref #50947, xref #58006

enforced deprecation of any/all with datetime64

@@ -1665,12 +1665,7 @@ def _groupby_op(
raise TypeError(f"datetime64 type does not support {how} operations")
if how in ["any", "all"]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you combine this if with the one above?

Copy link
Contributor Author

@natmokval natmokval Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sure. Done

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor suggestion on wording otherwise lgtm

FutureWarning,
stacklevel=find_stack_level(),
)
raise TypeError("datetime64 type does not support any operations")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raise TypeError("datetime64 type does not support any operations")
raise TypeError("datetime64 type does not support operation: 'all'")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be 'any' instead since this is for nanany?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea sorry - but my suggestion applies to both the any and all implementations. does not support (any|all) operations is ambiguous language whereas does not support operation: (any|all) is clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you, I agree, the new wording looks much better. I used the message "does not support (any|all) operations" because it was the original error message from _groupby_op and I didn't want to change it.

I corrected the messages as you suggested.

@mroeschke mroeschke merged commit a82ce84 into pandas-dev:main Mar 28, 2024
46 checks passed
@mroeschke
Copy link
Member

Thanks @natmokval

pmhatre1 pushed a commit to pmhatre1/pandas-pmhatre1 that referenced this pull request May 7, 2024
* enforce deprecation any/all with datetime64, correct tests

* add a note to v3.0.0

* enforce depr any/all with dt64 in groupby

* combine two if

* change wording in the error msg

* fix test_in_numeric_groupby

* fix test_cython_transform_frame_column
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Reduction Operations sum, mean, min, max, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants