Skip to content

Raise error when trying to construct time-zone aware timestamps - #13830

Merged
rapids-bot[bot] merged 5 commits into
NVIDIA:branch-23.10from
galipremsagar:13825
Aug 7, 2023
Merged

Raise error when trying to construct time-zone aware timestamps#13830
rapids-bot[bot] merged 5 commits into
NVIDIA:branch-23.10from
galipremsagar:13825

Conversation

@galipremsagar

Copy link
Copy Markdown
Contributor

Description

Fixes: #13825 This PR raises an error when a time-zone-aware scalar is passed to binops or cudf scalar construction.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@galipremsagar galipremsagar added bug Something isn't working 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. 4 - Needs cuDF (Python) Reviewer non-breaking Non-breaking change labels Aug 7, 2023
@galipremsagar galipremsagar self-assigned this Aug 7, 2023
@galipremsagar
galipremsagar requested a review from a team as a code owner August 7, 2023 20:05
Comment thread python/cudf/cudf/core/column/datetime.py Outdated
Co-authored-by: Ashwin Srinath <3190405+shwina@users.noreply.github.com>
Comment thread python/cudf/cudf/core/column/datetime.py Outdated
Comment thread python/cudf/cudf/core/column/timedelta.py Outdated
Comment thread python/cudf/cudf/tests/test_datetime.py Outdated
Comment thread python/cudf/cudf/tests/test_scalar.py Outdated
Comment thread python/cudf/cudf/tests/test_timedelta.py Outdated
Comment thread python/cudf/cudf/utils/dtypes.py Outdated
Comment thread python/cudf/cudf/core/column/datetime.py Outdated
Comment thread python/cudf/cudf/core/column/timedelta.py Outdated
Comment thread python/cudf/cudf/utils/dtypes.py Outdated
other = other.to_datetime64()
elif isinstance(other, pd.Timedelta):
other = other.to_timedelta64()
elif isinstance(other, datetime.datetime):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about TZ-aware datetime.datetime objects? e.g.

>>> import datetime
>>> datetime.datetime.now(datetime.timezone.utc)
datetime.datetime(2023, 8, 7, 20, 47, 48, 483493, tzinfo=datetime.timezone.utc)

Do those need to error, too? On conversion to np.datetime64, I get a warning. Is that sufficient?

>>> np.datetime64(datetime.datetime(2023, 8, 7, 20, 47, 48, 483493, tzinfo=datetime.timezone.utc))
<stdin>:1: DeprecationWarning: parsing timezone aware datetimes is deprecated; this will raise an error in the future
numpy.datetime64('2023-08-07T20:47:48.483493')

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we should raise for it too. Good catch, I thought datetime was tz agnostic.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bdice I updated the code to handle datetime.datetime objects aswell.

@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer labels Aug 7, 2023
@galipremsagar

Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot
rapids-bot Bot merged commit 9e099ce into NVIDIA:branch-23.10 Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working non-breaking Non-breaking change Python Affects Python cuDF API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Comparison with time-zone aware scalars should error

3 participants