Skip to content

BUG: Timestamp fails when fold is passed with positional args #52221

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

Closed
wants to merge 10 commits into from
Closed

BUG: Timestamp fails when fold is passed with positional args #52221

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9371f22
BUG: reformulate the predicate
AlexKirko Mar 26, 2023
b7b37ad
TST: add the test
AlexKirko Mar 26, 2023
e97541d
REFACT: mv from-positional inside the if
AlexKirko Mar 26, 2023
b3282a5
DOC: add whatsnew
AlexKirko Mar 26, 2023
6ee4770
attempt to fix the core issue instead
AlexKirko Mar 27, 2023
e0699ba
BUG: mv the bugfix to decorator to allow for mixed arguments
AlexKirko Mar 28, 2023
ebd419e
TST: update the tests now that behavior is consistent
AlexKirko Mar 28, 2023
870dc40
TST: add the argument mix test
AlexKirko Mar 28, 2023
635e0cc
CLN: clean up the function a bit
AlexKirko Mar 28, 2023
c942b70
Merge branch 'main' into gh-51117-positional-fold-bug
AlexKirko Mar 29, 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
1 change: 1 addition & 0 deletions doc/source/whatsnew/v2.1.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ Datetimelike
- Bug in :meth:`Timestamp.round` with values close to the implementation bounds returning incorrect results instead of raising ``OutOfBoundsDatetime`` (:issue:`51494`)
- :meth:`arrays.DatetimeArray.map` can now take a ``na_action`` argument. :meth:`DatetimeIndex.map` with ``na_action="ignore"`` now works as expected. (:issue:`51644`)
- Bug in :meth:`arrays.DatetimeArray.map` and :meth:`DatetimeIndex.map`, where the supplied callable operated array-wise instead of element-wise (:issue:`51977`)
- Bug in :class:`Timestamp` raising an error when passing fold when constructing from positional arguments.
-

Timedelta
Expand Down
8 changes: 8 additions & 0 deletions pandas/_libs/tslibs/timestamps.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1562,6 +1562,14 @@ class Timestamp(_Timestamp):
_TSObject ts
tzinfo_type tzobj

# GH#52117 If we passed positional args, then _ts_input
# is now set to year and the other positional args
# are shifted to the left. Let's shift back
if (isinstance(ts_input, int) and
isinstance(year, int) and isinstance(month, int)):
ts_input, year, month, day, hour, minute, second, microsecond = (
Copy link
Member

Choose a reason for hiding this comment

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

Would this be incorrect if a user passed Timestamp(2023, 1, 1, minute=1) by assigning minute to second?

Copy link
Member Author

@AlexKirko AlexKirko Mar 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, this is a good point. We get Timestamp('2023-01-01 00:00:01').

I hate it, but should I roll back to the bugfix version where I didn't touch the args, but instead just checked whether the first three args were filled or not and prevented the raise?

# GH#52117 note that currently we end up with the year in the ts_input
            # variable when using positional arguments. We should fix this.
            from_positional = (ts_input is not None and
                               year is not None and month is not None)

            if (ts_input is not _no_input and not from_positional and not (
                    PyDateTime_Check(ts_input) and
                    getattr(ts_input, "tzinfo", None) is None)):
                raise ValueError(
                    "Cannot pass fold with possibly unambiguous input: int, "
                    "float, numpy.datetime64, str, or timezone-aware "
                    "datetime-like. Pass naive datetime-like or build "
                    "Timestamp from components."
                )

Another solution would be to make all the args up to second positional-only, since Python 3.8 allows that, but it would deviate from the datetime API and be a breaking change. That would need a separate PR and a deprecation cycle, I think.

Copy link
Member Author

@AlexKirko AlexKirko Mar 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, there might be a different (non-breaking, non-horrible) solution, give me a day or so to try something out.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I would suggest tackling the init argument complexity in a follow up unless it's a truly a blocker for this fix

Copy link
Member Author

Choose a reason for hiding this comment

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

@mroeschke Thanks for giving that example. Turns out, there is another bug caused by the same anomaly in the data. We currently have on main:

pd.Timestamp(2020, 12, 31, second=1)

> Timestamp('2020-12-31 00:00:00.000001')

This looks like a bug to me. In addition, we are testing for this unexpected behavior here:

    @pytest.mark.parametrize("kwd", ["nanosecond", "microsecond", "second", "minute"])
    def test_constructor_positional_keyword_mixed_with_tzinfo(self, kwd, request):
        # TODO: if we passed microsecond with a keyword we would mess up
        #  xref GH#45307
        if kwd != "nanosecond":
            # nanosecond is keyword-only as of 2.0, others are not
            mark = pytest.mark.xfail(reason="GH#45307")
            request.node.add_marker(mark)

        kwargs = {kwd: 4}
        ts = Timestamp(2020, 12, 31, tzinfo=timezone.utc, **kwargs)

        td_kwargs = {kwd + "s": 4}
        td = Timedelta(**td_kwargs)
        expected = Timestamp("2020-12-31", tz=timezone.utc) + td
        assert ts == expected

My proposed fix makes it so that the fails here no longer happen and that:

pd.Timestamp(2020, 12, 31, second=1)

> Timestamp('2020-12-31 00:00:01')

Copy link
Member Author

@AlexKirko AlexKirko Mar 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I would suggest tackling the init argument complexity in a follow up unless it's a truly a blocker for this fix

That's one option but please review the current proposed solution. I think it's not terribly costly and makes the constructor easier to maintain and less vulnerable to bugs.

_no_input, ts_input, year, month, day, hour, minute, second)

_date_attributes = [year, month, day, hour, minute, second,
microsecond, nanosecond]

Expand Down
10 changes: 10 additions & 0 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 @@ -899,3 +899,13 @@ def test_timestamp_constructor_adjust_value_for_fold(tz, ts_input, fold, value_o
result = ts._value
expected = value_out
assert result == expected


@pytest.mark.parametrize("tz", ["dateutil/Europe/London"])
def test_timestamp_constructor_positional_with_fold(tz):
# Check that we build an object successfully
# if we pass positional arguments and fold
ts = Timestamp(2019, 10, 27, 1, 30, tz=tz, fold=0)
result = ts._value
expected = 1572136200000000
assert result == expected