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.

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

Add support for datetime64 #334

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

hhaensel
Copy link

@hhaensel hhaensel commented Jul 1, 2023

This PR tentatively addresses #293

@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2023

Thank you for your pull request. It has been marked as stale because it has been open for 60 days with no activity. If the PR is still relevant then please leave a comment, or else it will be closed in 30 days.

@github-actions github-actions bot added the stale Issues about to be auto-closed label Sep 4, 2023
@hhaensel
Copy link
Author

hhaensel commented Sep 4, 2023

I think, this is still relevant.
@cjdoris if you think some modifications are necessary, please let me know.

@github-actions github-actions bot removed the stale Issues about to be auto-closed label Sep 5, 2023
@cjdoris
Copy link
Collaborator

cjdoris commented Sep 13, 2023

Thanks for the PR! A couple of comments:

PythonCall avoids relying on anything other than the Python standard library - so Py(::Period) should not be converted to a numpy.timedelta64 (if anything it should be a datetime.timdelta). No objection to the reverse conversion though.

However, there are differing semantics going on. Both datetime.timedelta and numpy.timedelta64 represent fixed periods of time, so 1 hour is always 3600 seconds. However a Julia Dates.Period represents a calendar period of time, whose precise length is context dependent, so for example 1 hour can be 3600 or 3601 seconds depending on if the hour has a leap second or not. Therefore it doesn't really make sense to convert between them. It might be OK to convert small units (seconds, milliseconds, etc) because these have a fixed length but (a) this feels like a special case and (b) maybe one day we'll add leap milliseconds to the calendar.

@hhaensel
Copy link
Author

hhaensel commented Oct 1, 2023

PythonCall avoids relying on anything other than the Python standard library - so Py(::Period) should not be converted to a numpy.timedelta64 (if anything it should be a datetime.timdelta). No objection to the reverse conversion though.

Sounds like a good proposal.

However a Julia Dates.Period represents a calendar period of time

I see, I was not aware of the Period semantics in Julia, but it totally makes sense. I think as soon as we leave out Year everything should be fine. At least Julia converts happily between the lower units Week, Hour, Day, Second, etc ... while it throws an error as soon as Year is involved.

Should I wait for the refactoring, before continuing with this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants