-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
BUG: clip raising with na series bounds for datetimes or ea int #45161
Conversation
phofl
commented
Jan 1, 2022
- closes BUG: Confusing error message when clipping datetimes #44785
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
if is_datetime64_any_dtype(self.dtype): | ||
fill_value = Timestamp.max if method_name_le else Timestamp.min | ||
elif is_extension_array_dtype(self.dtype): | ||
fill_value = self.max() if method_name_le else self.min() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better way to do this for ea ints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pandas/core/dtypes/missing/na_value_for_dtype
pandas/core/generic.py
Outdated
fill_value = np.inf if method.__name__ == "le" else -np.inf | ||
method_name_le = method.__name__ == "le" | ||
if is_datetime64_any_dtype(self.dtype): | ||
fill_value = Timestamp.max if method_name_le else Timestamp.min |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for dt64tz shouldn't fill_value be tzaware?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to create a timezone aware Timestamp ts where ts > Timestamp.max would be true?
The fill value is not show in the result, it is just a placeholder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to create a timezone aware Timestamp ts where ts > Timestamp.max would be true?
no, that comparison would always raise TypeError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm ok, then we could use the max case for timzeones too
@@ -749,6 +749,7 @@ Numeric | |||
- Bug in :class:`DataFrame` arithmetic ops with a subclass whose :meth:`_constructor` attribute is a callable other than the subclass itself (:issue:`43201`) | |||
- Bug in arithmetic operations involving :class:`RangeIndex` where the result would have the incorrect ``name`` (:issue:`43962`) | |||
- Bug in arithmetic operations involving :class:`Series` where the result could have the incorrect ``name`` when the operands having matching NA or matching tuple names (:issue:`44459`) | |||
- Bug in :meth:`Series.clip` raising if bounds are a :class:`Series` with ``NA`` values for datetimes or nullable integer dtypes (:issue:`44785`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you move to 1.5
if is_datetime64_any_dtype(self.dtype): | ||
fill_value = Timestamp.max if method_name_le else Timestamp.min | ||
elif is_extension_array_dtype(self.dtype): | ||
fill_value = self.max() if method_name_le else self.min() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pandas/core/dtypes/missing/na_value_for_dtype
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
@phofl can you rebase |
Sorry, forgot. Will do in the coming days |
Thanks for the pull request, but it appears to have gone stale. Feel free to update and reopen when you have the time, but closing to clear out the queue |