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.

Sign up for GitHub

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: zero-pad shorter years in Timestamp.isoformat #52220

Merged
merged 5 commits into from
Mar 31, 2023
Merged

BUG: zero-pad shorter years in Timestamp.isoformat #52220

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eb651c2
add tests checking that non-4 digit years isoformat properly
keewis Mar 24, 2023
1ccf530
zero-pad the year to 4 digits
keewis Mar 24, 2023
7450c13
adapt the constructor consistency tests
keewis Mar 26, 2023
e5f7b48
remove the constructor error message
keewis Mar 26, 2023
ab87865
Merge branch 'main' into isoformat-years
keewis Mar 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pandas/_libs/tslibs/timestamps.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1015,7 +1015,7 @@ cdef class _Timestamp(ABCTimestamp):
base_ts = "microseconds" if timespec == "nanoseconds" else timespec
base = super(_Timestamp, self).isoformat(sep=sep, timespec=base_ts)
# We need to replace the fake year 1970 with our real year
base = f"{self.year}-" + base.split("-", 1)[1]
base = f"{self.year:04d}-" + base.split("-", 1)[1]

if self.nanosecond == 0 and timespec != "nanoseconds":
return base
Expand Down
13 changes: 5 additions & 8 deletions pandas/tests/scalar/timestamp/test_constructors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,19 +595,16 @@ def test_out_of_bounds_string_consistency(self, arg):
# GH 15829
msg = "|".join(
[
"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",
Copy link
Contributor

@spencerkclark spencerkclark Mar 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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 the as_unit call. 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 its unit.
  • For "0001-01-01" a second precision Timestamp with year equal one was created, but when attempting to convert to nanosecond precision with as_unit the "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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I removed the check for the Out of bounds message

]
)
with pytest.raises(OutOfBoundsDatetime, match=msg):
Timestamp(arg).as_unit("ns")

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
Comment on lines -611 to +608
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

This was fixed in #51039

https://www.kaggle.com/code/marcogorelli/pandas-regression-example?scriptVersionId=124051955


def test_min_valid(self):
# Ensure that Timestamp.min is a valid Timestamp
Expand Down
11 changes: 11 additions & 0 deletions pandas/tests/scalar/timestamp/test_formats.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@
second=8,
microsecond=132263,
)
ts_no_ns_year1 = Timestamp(
year=1,
month=5,
day=18,
hour=15,
minute=17,
second=8,
microsecond=132263,
)
ts_ns = Timestamp(
year=2019,
month=5,
Expand Down Expand Up @@ -50,6 +59,8 @@
(ts_no_ns, "auto", "2019-05-18T15:17:08.132263"),
(ts_no_ns, "seconds", "2019-05-18T15:17:08"),
(ts_no_ns, "nanoseconds", "2019-05-18T15:17:08.132263000"),
(ts_no_ns_year1, "seconds", "0001-05-18T15:17:08"),
(ts_no_ns_year1, "nanoseconds", "0001-05-18T15:17:08.132263000"),
(ts_ns, "auto", "2019-05-18T15:17:08.132263123"),
(ts_ns, "hours", "2019-05-18T15"),
(ts_ns, "minutes", "2019-05-18T15:17"),
Expand Down