Skip to content

Conversation

@hhaensel
Copy link
Contributor

This PR tentatively addresses #293

@github-actions
Copy link
Contributor

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-actionsgithub-actionsbot added the stale Issues about to be auto-closed label Sep 4, 2023
@hhaensel
Copy link
ContributorAuthor

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

@github-actionsgithub-actionsbot removed the stale Issues about to be auto-closed label Sep 5, 2023
@cjdoris
Copy link
Member

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
ContributorAuthor

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?

@hhaensel
Copy link
ContributorAuthor

Close in favour of #509

Sign up for freeto 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

@hhaensel@cjdoris