API: Period.to_timestamp default to microsecond unit#63760
Conversation
jorisvandenbossche
added
Period
jorisvandenbossche
left a comment
jorisvandenbossche
left a comment
There was a problem hiding this comment.
Yes, I think that would be good!
| elif freq == FR_DAY: | ||
| dta = periodarr.base.view("M8[D]") | ||
| return astype_overflowsafe(dta, dtype=DT64NS_DTYPE) | ||
| # GH#??? give microseconds for everything other than freq="ns" |
There was a problem hiding this comment.
| # GH#??? give microseconds for everything other than freq="ns" | |
| # give microseconds for everything other than freq="ns" |
There was a problem hiding this comment.
will update with the GH ref
| return self.to_timestamp(how="start") + adjust | ||
| else: | ||
| adjust = Timedelta(1, "ns") | ||
| adjust = Timedelta(1, unit=unit).as_unit(unit) |
There was a problem hiding this comment.
| adjust = Timedelta(1, unit=unit).as_unit(unit) | |
| adjust = Timedelta(1, unit=unit) |
Is that still needed now? (I thought we follow the input unit for integers)
There was a problem hiding this comment.
i think you're right, will update
| result = obj.to_timestamp("H", "end") | ||
| exp_index = _get_with_delta(delta) | ||
| exp_index = exp_index + Timedelta(1, "h") - Timedelta(1, "ns") | ||
| exp_index = exp_index + np.timedelta64(1, "h") - np.timedelta64(1, "us") |
There was a problem hiding this comment.
Is there a specific reason you are switching here from Timedelta to np.timedelta64 ?
There was a problem hiding this comment.
i think i started this branch before we changed the Timedelta behavior. will revert this bit
There was a problem hiding this comment.
(the same change is used in some other files as well)
| @@ -42,10 +41,8 @@ def test_to_timestamp_out_of_bounds(self): | |||
| # GH#19643, used to incorrectly give Timestamp in 1754 | |||
| with tm.assert_produces_warning(FutureWarning, match=bday_msg): | |||
| per = Period("0001-01-01", freq="B") | |||
There was a problem hiding this comment.
If you would change the freq to use ns, then it would keep raising the out of bounds?
There was a problem hiding this comment.
do you mean the freq we pass to the Period constructor in this example? that would raise at construction.
There was a problem hiding this comment.
Ah, yes, that's what I meant, but indeed not helping to test this
|
Some typing failures |
|
Thanks @jbrockmendel! |
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
2 participants
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.AGENTS.md.cc @jorisvandenbossche do we want to get this in under the wire?