-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
BUG: Fix dt64[non_nano] + some_offsets incorrectly rounding #62383
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
base: main
Are you sure you want to change the base?
Conversation
pandas/core/arrays/datetimes.py
Outdated
|
||
else: | ||
result = type(self)._simple_new(res_values, dtype=res_values.dtype) | ||
units = ["ns", "us", "ms", "s", "m", "h", "D"] |
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.
are m, h, D possible?
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.
No, I'm wrong I will update this .
pandas/core/arrays/datetimes.py
Outdated
idx_self = units.index(self.unit) | ||
idx_offset = units.index(offset_unit) | ||
res_unit = units[min(idx_self, idx_offset)] | ||
dtype = tz_to_dtype(self.tz, unit=res_unit) |
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.
wont you need to cast res_values before calling simple_new?
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.
yes, thanks! . Should we wrap this in try/except
block?
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.
i suspect that The Right Way to do this would be to find the correct dtype before calling _from_sequence on L817 and pass it there
pandas/core/arrays/datetimes.py
Outdated
result = type(self)._simple_new(res_values, dtype=dtype) | ||
offset_td = Timedelta(offset.offset) | ||
offset_unit = offset_td.unit | ||
if self.unit in units and offset_unit in units: |
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 ever False?
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.
no
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.
Then the check is not necessary
pandas/core/arrays/datetimes.py
Outdated
self.tz is not None | ||
and getattr(result.dtype, "tz", None) is None | ||
and res_unit == "ns" | ||
): |
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.
why does this codnition need tom change?
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.
it prevents errors by making sure we only localize when it's valid, please correct me
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.
Well result.dtype.tz is always None, so that part of the check is unnecessary. And I suspect the u it part is just wrong.
pandas/core/arrays/datetimes.py
Outdated
idx_self = units.index(self.unit) | ||
idx_offset = units.index(offset_unit) | ||
res_unit = units[min(idx_self, idx_offset)] | ||
dtype_naive = np.dtype(f"datetime64[{res_unit}]") |
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.
instead of defining dtype_naive and doing astype, could you do .as_unit(res_unit) after simple_new?
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.
thanks!
This PR updates logic selects the finest unit between the array and the offset, preventing rounding errors. A regression test is included to confirm correct behavior for affected offsets and units.
Please let me know if my approach or fix needs any improvements . I’m open to feedback and happy to make changes based on suggestions.
Thankyou !