JuliaStats / TimeSeries.jl

Time series toolkit for Julia
Other
349 stars 69 forks source link

collapse (document) does not work correctly #498

Closed clouds56 closed 3 years ago

clouds56 commented 3 years ago

As document https://juliastats.org/TimeSeries.jl/latest/combine/#collapse-1 Considering following code

dates = [Date(2018, 1, 1), Date(2018, 1, 2), Date(2019, 1, 1), Date(2019, 2, 1)]
ta = TimeArray(dates, rand(length(dates)))
collapse(ta, month, last)

gives only 2 entries

│            │ A      │
├────────────┼────────┤
│ 2019-01-01 │ 0.405  │
│ 2019-02-01 │ 0.9778 │

while I expects 3 entries

│            │ A      │
├────────────┼────────┤
│ 2018-01-02 │ 0.9279 │
│ 2019-01-01 │ 0.405  │
│ 2019-02-01 │ 0.9778 │

and I have to write collapse(ta, x->floor(x, Month(1)), last). shall we change the api to collapse(ta, Month(1), last) to reduce confusion, just like what https://github.com/femtotrader/TimeSeriesResampler.jl do?

iblislin commented 3 years ago

while I expects 3 entries

ah, that's kind of pitfall. Maybe we can covert all day,month, ...etc functions to x->floor(x, ...) in our collapse function.

shall we change the api to collapse(ta, Month(1), last) to reduce confusion, just like what https://github.com/femtotrader/TimeSeriesResampler.jl do?

hmm, just a collapse(ta, ::Period, ...) right? Are there other APIs you want?

clouds56 commented 3 years ago

BTW, I do not want collapse perverse parameters D, by writing

collapse(ta, x->floor(x, Day(1)), Date ∘ first, sum)

I expect a transform from TimeArray{T, N, DateTime} to TimeArray{T, N, Date}. Now I have to decompose TimeArray and construct a new one. (The map also keeps D)

iblislin commented 3 years ago

I expect a transform from TimeArray{T, N, DateTime} to TimeArray{T, N, Date}.

Converting DateTime to Date is kind of shrinking the ability, like idea of arithmetic operations usually promote type from Int to Float but not Float to Int. I don't think this API should convert it implicitly. Maybe we can provide other handy API to let user do it.

clouds56 commented 3 years ago

I don't think it's that implicit, in contrast, I think it's explicit. Note the Date in the expression collapse(ta, x->floor(x, Day(1)), Date ∘ first, sum). I think the collapse is more like a map, that apply Date ∘ first to Vector{D} and sum to Matrix{T}.

iblislin commented 3 years ago

ah, that sounds more reasonable. I'm going to make several PRs for this issue.