-
Notifications
You must be signed in to change notification settings - Fork 605
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.
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
fix(datatypes): return pd.Timestamp
or pd.Series[datetime64]
for date.to_pandas()
#8784
base: main
Are you sure you want to change the base?
fix(datatypes): return pd.Timestamp
or pd.Series[datetime64]
for date.to_pandas()
#8784
Conversation
d3643b5
to
79ce1c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach looks reasonable, the devil is in the details as can be seen in CI 😂
@@ -74,7 +74,7 @@ def convert_Date(cls, s, dtype, pandas_type): | |||
else: | |||
s = dd.to_datetime(s) | |||
|
|||
return s.dt.normalize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think might want to keep this, but not 100% sure. In theory once you're in this function you shouldn't need to normalize ... in theory :)
# not run the tests on this backend. If we do, should we also | ||
# modify `convert_Timestamp_element()` and | ||
# `convert_Time_element()` similarly? | ||
return pd.Timestamp.fromisoformat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is sort of up for grabs given that pandas doesn't have a standard way to represent an array of dates (the _element
suffix implies [perhaps not in an obvious way] that this function is being called once per element of an array).
I think it's fine to also change this to using pandas timestamps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach looks reasonable, the devil is in the details as can be seen in CI 😂
55b3c7a
to
3271f89
Compare
pd.Timestamp
or pd.Series[datetime64]
for date.to_pandas()
[WIP]pd.Timestamp
or pd.Series[datetime64]
for date.to_pandas()
3271f89
to
d1b4cf6
Compare
@mfatihaktas What's the status of this one? would you mind fixing the conflicts and triggering CI to see if things are green and request a review. |
Thanks for checking on it. Regarding the status, I marked this PR as ready-for-review some time ago. I am busy with another project this week, but will try to resolve the conflicts before EOD on Friday. |
d1b4cf6
to
de1fe8a
Compare
de1fe8a
to
440d7a8
Compare
I think this PR is good to go, but it's slated for 10.0, so please do not merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DO NOT MERGE
pd.Timestamp
or pd.Series[datetime64]
for date.to_pandas()
pd.Timestamp
or pd.Series[datetime64]
for date.to_pandas()
I made the PR title non-compliant with conventional commits, so we can avoid accidentally clicking the big green button. |
ACTION NEEDED Ibis follows the Conventional Commits specification for release automation. The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. |
pd.Timestamp
or pd.Series[datetime64]
for date.to_pandas()
pd.Timestamp
or pd.Series[datetime64]
for date.to_pandas()
Description of changes
Aims to close #8019.
The example given by @NickCrews in the Issue description runs as follows with the changes here:
To the reviewer: @NickCrews added the following suggestion in the issue:
Should we add new tests to verify these suggestions?