BUG: zero-pad shorter years in Timestamp.isoformat
#52220
Conversation
| "Cannot cast 1-01-01 00:00:00 to unit='ns' without overflow", | ||
| "Out of bounds nanosecond timestamp: 1-01-01 00:00:00", | ||
| "Cannot cast 0001-01-01 00:00:00 to unit='ns' without overflow", | ||
| "Out of bounds nanosecond timestamp: 0001-01-01 00:00:00", |
There was a problem hiding this comment.
To elaborate on my comment in the issue, my guess is that it used to be that different error messages were raised for the different input options in this test:
- For
"001-01-01"the Timestamp constructor assumed nanosecond precision and so would not allow the construction of a nanosecond Timestamp with year equal one, raising the"Out of bounds nanosecond timestamp: 0001-01-01 00:00:00"message before one could try theas_unitcall. This necessitated the if-statement below, because it meant it was not possible to create a Timestamp object with"001-01-01"(i.e. previously line 608) without raising an error, so one could not query itsunit. - For
"0001-01-01"a second precision Timestamp with year equal one was created, but when attempting to convert to nanosecond precision withas_unitthe"Cannot cast 0001-01-01 00:00:00 to unit='ns' without overflow"message was raised.
Now that the Timestamp constructor appears to assume second precision for "001-01-01" as well, the "Out of bounds nanosecond timestamp: 0001-01-01 00:00:00" is no longer raised for either of the two test cases, so the matching condition can be simplified to be the "Cannot cast 0001-01-01 00:00:00 to unit='ns' without overflow" message alone.
There was a problem hiding this comment.
thanks, I removed the check for the Out of bounds message
since that should not be raised anymore
|
I wonder if it would be possible to include this in |
There was a problem hiding this comment.
Nice! cc @jbrockmendel in case there's comments
Agree that this small and affects a new 2.0 feature, so I'd be OK with backporting and including in 2.0
| if arg == "0001-01-01": | ||
| # only the 4-digit year goes through ISO path which gets second reso | ||
| # instead of ns reso | ||
| ts = Timestamp(arg) | ||
| assert ts.unit == "s" | ||
| assert ts.year == ts.month == ts.day == 1 | ||
| ts = Timestamp(arg) | ||
| assert ts.unit == "s" | ||
| assert ts.year == ts.month == ts.day == 1 |
There was a problem hiding this comment.
nice! and also a reminder that if we have an if in a test, we should probably be explicit about the else, else it gets forgotten and stays in forever
I'm having a look and when this was fixed
There was a problem hiding this comment.
|
Thanks @keewis |
…imestamp.isoformat`) (#52327) Backport PR #52220: BUG: zero-pad shorter years in `Timestamp.isoformat` Co-authored-by: Justus Magin <[email protected]>
* add tests checking that non-4 digit years isoformat properly * zero-pad the year to 4 digits * adapt the constructor consistency tests * remove the constructor error message since that should not be raised anymore
As discussed in #50867, this changes
Timestamp.isoformatto zero-pad years to 4 digits.After applying the change, I also had to change a existing test because of the now-changed standard format, but also remove a condition which assumed that
"001-01-01"and"0001-01-01"would result in different objects (not sure, maybe there was a time where the resultingTimestampobject would not be equal?). However, on currentmain:cc @spencerkclark
No
whatsnewentry yet because I was not sure where to put it.Added type annotations to new arguments/methods/functions.doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.