Add timedelta, timedelta64 and datetime64 plus respective conversions#509

hhaensel · 2024-06-16T22:34:33Z

This PR replaces #334 and takes into account the major refactoring of PythonCall.
Particularly, it fixes#293.

What's new?

Python Constructors

julia>pytimedelta(hour =1, minute =2) Python: datetime.timedelta(seconds=3720) julia>pytimedelta64(hour =1, minute =2) Python: numpy.timedelta64(62,'m') julia>pytimedelta64(year =2, month =3) Python: numpy.timedelta64(27,'M') julia>pydatetime64(year =2024, month =3) Python: numpy.datetime64('2024-03-01T00:00:00')

Conversion to Julian types

julia> x =pytimedelta64(year =11) Python: numpy.timedelta64(11,'Y') julia>pyconvert(Any, x) |> x -> (x, typeof(x)) (11 years, Dates.CompoundPeriod) julia>pyconvert(Period, x) |> x -> (x, typeof(x)) (Year(11), Year)

DataFrame handling

I've set the priority of datetime, timedelta, datetime64 and timedelta64 to ARRAY, which allows for automatic Table conversion - I hope that's the intended way to do it.

julia> jdf =DataFrame(x = [now() +Second(rand(1:1000)) for _ in1:100], y = [Second(n) for n in1:100]) 100×2 DataFrame Row │ x y │ DateTime Second ─────┼────────────────────────────────────── 1 │ 2024-06-17T00:31:31.2361 second 2 │ 2024-06-17T00:30:30.2362 seconds 3 │ 2024-06-17T00:41:22.2363 seconds ⋮ │ ⋮⋮98 │ 2024-06-17T00:36:05.23698 seconds 99 │ 2024-06-17T00:38:38.23699 seconds 100 │ 2024-06-17T00:28:21.236100 seconds 94 rows omitted julia> pdf =pytable(jdf) Python: x y 02024-06-1700:31:31.2360 days 00:00:0112024-06-1700:30:30.2360 days 00:00:0222024-06-1700:41:22.2360 days 00:00:0332024-06-1700:33:52.2360 days 00:00:04...4 more lines ...972024-06-1700:36:05.2360 days 00:01:38982024-06-1700:38:38.2360 days 00:01:39992024-06-1700:28:21.2360 days 00:01:40 [100 rows x 2 columns] julia>DataFrame(PyTable(pdf)) 100×2 DataFrame Row │ x y │ DateTime Compound… ─────┼─────────────────────────────────────────────── 1 │ 2024-06-17T00:31:31.2361 second 2 │ 2024-06-17T00:30:30.2362 seconds 3 │ 2024-06-17T00:41:22.2363 seconds ⋮ │ ⋮⋮98 │ 2024-06-17T00:36:05.2361 minute, 38 seconds 99 │ 2024-06-17T00:38:38.2361 minute, 39 seconds 100 │ 2024-06-17T00:28:21.2361 minute, 40 seconds 94 rows omitted

Default Conversion

I chose to use Dates.CompoundPeriod as result type of default conversion from timedelta64 as both types support year, month and minor period units. This is debatable, we could also change it to Period, hence the resulting type would depend on the input.

julia>pyconvert(Any, x) |> x -> (x, typeof(x)) (11 years, Dates.CompoundPeriod) julia>pyconvert(Period, x) |> x -> (x, typeof(x)) (Year(11), Year)

Both Python and Julia do not convert between Year/Month and the other period types, so there is no danger with this choice to arrive at ill-determined intervals.
The difference is that Julia allows addition/subtraction of mixed types while Python/Numpy throws an error.

The difference to the previous PR is that all conversions rely on either builtin or numpy functions and do not use pandas.

Ordering of arguments for pytimedelta() was chosen to be identical to the python version, while ordering for pytimedelta64() is strictly descending, except week which comes last.

EDIT: add comments in what's new code, added conversions
EDIT2: removed comment about datetime_data, I had misunderstood the meaning and have updated the code

hhaensel · 2024-06-27T21:19:47Z

@cjdoris what do you think about this PR?

hhaensel · 2024-08-15T16:04:21Z

@cjdoris still hoping to get this integrated ...
Is there anything pending or unclear where I could support?

MilesCranmer · 2024-08-23T04:52:49Z

Just chiming in with some comments:

1: Could you add some tests for this? This adds a lot of new features so I think should have testing to cover everything. Ideally the test coverage of the diff should be 100%. It should also cover your intended usecase with DataFrame. It should also cover the behavior about:

The difference is that Julia allows addition/subtraction of mixed types while Python/Numpy throws an error.

as this seems subtle.

2: Is the 3-arg version of the @py needed? I don't see it being used anywhere. I think it should go into a different PR.

src/Convert/numpy.jl

MilesCranmer · 2024-08-23T05:01:56Z

src/Convert/numpy.jl

+ year::Int=_year, month::Int=_month, day::Int=_day, hour::Int=_hour, minute::Int=_minute, second::Int=_second,
+ millisecond::Int=_millisecond, microsecond::Int=_microsecond, nanosecond::Int=_nanosecond
+)
+ pyimport("numpy").datetime64("$(DateTime(year, month, day, hour, minute, second))") + pytimedelta64(;millisecond, microsecond, nanosecond)


Is pyimport("numpy") the correct API call, or is that just to be used in user packages?

I saw similar calls at different places in the package, so I took this approach. But I also wouldn't know how to code a timedelta64 without calling pyimport.
Please let me know if there's a better solution.

src/Convert/numpy.jl

MilesCranmer · 2024-08-23T05:03:34Z

src/Convert/numpy.jl

+ T = types[findfirst(==(unit), units)]
+ pyconvert_return(CompoundPeriod(T(value * count)) |> canonicalize)


The proper way to do this would be to use Base.Cartesian.@nif. That way you could write this code to avoid dynamic dispatch on types (which will be very slow).

MilesCranmer · 2024-08-23T05:04:27Z

src/Convert/numpy.jl

+ units = ("Y", "M", "W", "D", "h", "m", "s", "ms", "us", "ns")
+ types = (Year, Month, Week, Day, Hour, Minute, Second, Millisecond, Microsecond, Nanosecond)
+ T = types[findfirst(==(unit), units)]
+ pyconvert_return(DateTime(_base_datetime) + T(value * count))


Similar to other comment – you should write this using Base.Cartesian.@nif over the types tuple to avoid dynamic dispatch.

Again, tested with a julia function calls and found that the julia part is around 25ns, whereas the python call is around 1.5microsecond

src/Convert/numpy.jl

MilesCranmer · 2024-08-23T05:07:34Z

src/Convert/numpy.jl

+ _year::Int=0, _month::Int=0, _day::Int=0, _hour::Int=0, _minute::Int=0, _second::Int=0, _millisecond::Int=0, _microsecond::Int=0, _nanosecond::Int=0, _week::Int=0;
+ year::Int=_year, month::Int=_month, day::Int=_day, hour::Int=_hour, minute::Int=_minute, second::Int=_second, microsecond::Int=_microsecond, millisecond::Int=_millisecond, nanosecond::Int=_nanosecond, week::Int=_week)
+ pytimedelta64(sum((
+ Year(year), Month(month), # you cannot mix year or month with any of the below units in python, the error will be thrown by `pytimedelta64(::CompoundPeriod)`


This comment
you cannot mix year or month with any of the below units in python, the error will be thrown by pytimedelta64(::CompoundPeriod)
Should be presented to the user as a descriptive error message rather than a comment in the function

Maybe the comment isn't clear enough.
Python throws a well understandable descriptive error in case of wrong usage, so no need for us to do so. Agree?

MilesCranmer · 2024-08-23T05:08:16Z

src/Convert/numpy.jl

+
+function pyconvert_rule_timedelta64(::Type{CompoundPeriod}, x::Py)
+ unit, count = pyconvert(Tuple, pyimport("numpy").datetime_data(x))
+ value = reinterpret(Int64, pyconvert(Vector, x))[1]


Is reinterpret safe here? Is there a better alternative to use?

I thought, pyconvert creates a new Julia Vector which is not mapped onto Python data. If that would be the case, we'd need to wrap the vector by a copy().

MilesCranmer · 2024-08-23T05:09:24Z

src/Convert/numpy.jl

+for T in (CompoundPeriod, Year, Month, Day, Hour, Minute, Second, Millisecond, Microsecond, Nanosecond, Week)
+ pyconvert_add_rule("numpy:timedelta64", T, pyconvert_rule_timedelta64, priority)
+end


Since Julia is unlikely to unroll this loop, you should use Base.Cartesian.@nexprs here to avoid dynamic dispatch.

Tried my best, but I'm not sure how to test whether this will speed up things

MilesCranmer · 2024-08-23T05:13:39Z

src/Convert/numpy.jl

+ args = T .== (Day, Second, Millisecond, Microsecond, Minute, Hour, Week)
+ pydatetime64(x.value .* args...)


Probably better to rewrite this with Base.Cartesian.@nif rather than doing a masked sum, since you know there will be only 1 element in the sum.

This sum is dammed fast (16ns), and I couldn't beat it with a different version.

hhaensel · 2024-08-23T13:01:33Z

Thanks for all the comments, I already learned some new things about Julia, which is always nice.

I will go through them the next days and once we've agreed on the solutions write the test functions.

Just want to re-raise my question from the previous PR.

Do you think, we should stick with CompoundPeriod as default conversion type?
Alternatively, we could go for Period instead.

Co-authored-by: Miles Cranmer <miles.cranmer@gmail.com>

MilesCranmer · 2025-01-19T23:02:28Z

I see, thanks. In the meantime can you add tests and docs for the code changes? Ideally we should have 100% test coverage.

hhaensel · 2025-01-19T23:12:46Z

I did add tests for pytimedelta.
Tests of pytimedelta64 depend on numpy. But as far as I could see, you did not define a CondaPkg environment so that numpy or pandas could be tested. Moreover, I saw that none of the standard python API is documented by docstrings, e.g. pytime().
Therefore, I am a bit lost where to begin adding docstrings for my extensions.
I tested using

using CondaPk CondaPkg.add("pandas")

which I think is a reasonable approach.
If you are of the same opinion, I could write a PR that fixes the two locations, where :pandas is missing in the current tests and then continue completing this PR so that finally it doesn't error anymore.

MilesCranmer · 2025-01-20T02:31:54Z

Were the test files not committed? I don’t see any added tests

hhaensel · 2025-01-20T10:31:30Z

I submitted #588 and #589. I propose to merge these first before continuing here with docstrings and tests.

hhaensel · 2025-01-20T13:24:50Z

When writing the tests for pytimedelta and pytimedelta64 I realised that my choice for keyword argument naming might be confusing for users.
Currently it is

pytimedelta(second =1)

but

pyimport("datetime").timedelta(seconds =1)

My choice was inspired from the Dates API (e.g. Second(1))
I tend to append an 's' to the keywords, what do you think?

append 's' to keywords in pytimedelta and pytimedelta64

…a64 in DataFrames

hhaensel · 2025-01-20T15:26:18Z

Changed the keywords. Tests should pass as soon as the other two PRs are merged.
One missing error is Aqua's complaining about REPL being a stale dependency.

…me}), remove pydatetime64(::CompoundPeriod)

…nicalize, add global setting CANONICALIZE_TIMEDELTA64 for rule conversion

hhaensel · 2025-01-21T08:55:31Z

Added functionality that the unit of pytimedelta64() is defined by the least unit specified, also for the case 0.
Moreover, the user can specify whether time difference should be canonicalized, default is false.
So

julia> pytimedelta64(Year(0)) Python: np.timedelta64(0,'Y') julia> pytimedelta64(Week(0)) Python: np.timedelta64(0,'W') julia> pytimedelta64(Millisecond(0)) Python: np.timedelta64(0,'ms')

and

julia> pyconvert(Dates.CompoundPeriod, pytimedelta64(Second(60))) 60 seconds julia> pyconvert(Dates.CompoundPeriod, pytimedelta64(Second(60), canonicalize =true)) 1 minute

and

julia> pyconvert(Dates.CompoundPeriod, pytimedelta64(Second(60))) 60 seconds julia> PythonCall.Convert.CANONICALIZE_TIMEDELTA64[] =true true julia> pyconvert(Dates.CompoundPeriod, pytimedelta64(Second(60))) 1 minute

hhaensel · 2025-01-21T08:56:06Z

I hope you find these changes consistent.

hhaensel · 2025-01-21T17:05:10Z

There's an error for CondaPkg on Windows, but in principle, the tests pass.
Will try to find a better method for CondaPkg environment.

hhaensel · 2025-01-21T22:10:18Z

I cherry-picked #589 in order to proceed with testing.
Now everything seems fixed for julia 1.11

added tests for pytimedelta, pytimedelta64 and pydatetime64
several fixes that came up during testing
CondaPkg environment seems reasonably solved by moving to a test/Project.toml with its own CondaPkg.toml

The docs are probably the only remaining topic.
Should I simply add docstrings? What is your recommendation?

…pytimedelta64, pydatetime64

hhaensel · 2025-02-19T16:36:25Z

@cjdoris @MilesCranmer Ping!

Do you think we can finalize this PR?

Just pushed two (final?) commits so that we now have

removed @py method for python style imports (from datetime import datetime as dt) as there is already the julia-style import
reordered the arguments of pytimedelta() in decreasing order (except week, which omes last), while the original Python datetime.timedelta() function has a very unexpected signature that probably no one uses (pytimedeltatimedelta(days, seconds, microseconds, milliseconds, minutes, hours, weeks)). This way pytimedelta() has a similar signature to pytimedelta64()
tests included
docstrings included

Beforerr · 2025-03-04T20:32:50Z

Just friendly wondering why this could not be merged?

MilesCranmer · 2025-03-04T20:36:49Z

Would need @cjdoris to review first. I know he has been very busy lately, but I am sure he will review it as soon as he has time

hhaenseland others added 5 commits February 21, 2022 16:26

add support for @py from <module> import
093bdcb

Merge branch 'cjdoris:main' into main
c63ab23

Merge branch 'main' ofhttps://github.com/hhaensel/PythonCall.jl
5a65a52

support timedelta, timedelta64, datetime64 and respective conversions
5740b59

fix week kw in pytimedelta64, typo (space) in builtins
f897600

hhaensel changed the title ~~Add timedelta, timedelta64 and datetime64 plus respective conversions~~Add timedelta64 and datetime64 plus respective conversionsJun 17, 2024

hhaensel mentioned this pull request Jun 17, 2024
DataFrame(::PyPandasDataFrame) converts date & datetime to bytes #293
Open

hhaensel changed the title ~~Add timedelta64 and datetime64 plus respective conversions~~Add timedelta, timedelta64 and datetime64 plus respective conversionsJun 17, 2024

correct handling of count in 64-bit conversion rules
1e8d410

Merge remote-tracking branch 'origin/main' into hh-timedelta64
b3cc79f