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 _ in 1:100], y = [Second(n) for n in 1:100])
100×2 DataFrame
 Row │ x                        y
     │ DateTime                 Second
─────┼──────────────────────────────────────
   1 │ 2024-06-17T00:31:31.236  1 second
   2 │ 2024-06-17T00:30:30.236  2 seconds
   3 │ 2024-06-17T00:41:22.236  3 seconds
  ⋮  │            ⋮                  ⋮
  98 │ 2024-06-17T00:36:05.236  98 seconds
  99 │ 2024-06-17T00:38:38.236  99 seconds
 100 │ 2024-06-17T00:28:21.236  100 seconds
                             94 rows omitted

julia> pdf = pytable(jdf)
Python:
                         x               y
0  2024-06-17 00:31:31.236 0 days 00:00:01
1  2024-06-17 00:30:30.236 0 days 00:00:02
2  2024-06-17 00:41:22.236 0 days 00:00:03
3  2024-06-17 00:33:52.236 0 days 00:00:04
           ... 4 more lines ...
97 2024-06-17 00:36:05.236 0 days 00:01:38
98 2024-06-17 00:38:38.236 0 days 00:01:39
99 2024-06-17 00:28:21.236 0 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.236  1 second
   2 │ 2024-06-17T00:30:30.236  2 seconds
   3 │ 2024-06-17T00:41:22.236  3 seconds
  ⋮  │            ⋮                      ⋮
  98 │ 2024-06-17T00:36:05.236  1 minute, 38 seconds
  99 │ 2024-06-17T00:38:38.236  1 minute, 39 seconds
 100 │ 2024-06-17T00:28:21.236  1 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 <[email protected]>

hhaensel · 2024-09-06T23:43:59Z

Concerning the three-argument version of @py; it's probably best to remove it as it goes against the authors' initial idea of being as close as possible to Julia syntax, right?

hhaensel · 2024-09-27T05:52:35Z

@cjdoris @MilesCranmer
Apart from my question above this PR would be ready to merge from my end.
Please let me know if you have other concerns.
We may want to add testing, please also let me know.

hhaensel and 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' of https://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 conversions Jun 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 conversions Jun 17, 2024

correct handling of count in 64-bit conversion rules

1e8d410

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

b3cc79f