-
Notifications
You must be signed in to change notification settings - Fork 422
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
Hybrid date time index - Initial Proposal #88
base: master
Are you sure you want to change the base?
Conversation
a8e6268
to
d2c792a
Compare
Additions to the public API:
Additions to the private API:
|
Apologies for the delay in getting to this, Ahmed. I've been traveling and on vacation the last couple weeks, but I will make sure to look at this this week. |
I'm wondering if we can break this out into a few smaller changes? E.g. can HybridDateTimeIndex be its own change, the DateTimeIndex operations like union etc. be its own change, and the TimeSeries API additions be their own change? How difficult would it be to separate these out? Also, should |
If it is for readability, I've tried to organize commits so that it is easy to understand the changes going though commits one by one. If it is not doing, I'll break it up then. Please, let me know. I think yes |
Ah, yes, the commit history does look nice. My concern is that it'll take me a non-negligible amount of time to get through the whole patch, and if I have suggested revisions for minor changes, it'll hold the whole thing up. Where, if alternatively, it's broken up into a couple different changes, we can start merging parts of it sooner. Which will save you work rebasing if other things need to get merged in the mean time. Would you be open to submitting all the commits up to 066efe8 as a PR? |
|
||
private val sizeOnLeft: Array[Int] = { | ||
var sum: Int = 0 | ||
(Array(0) ++ indices.init.map(_.size)).map {a => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: add space after curly brace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could probably be written a little more clearly with scanLeft
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea 👍
I got your point. I've opened a new PR #96 for the |
Comparing DateTimeIndex instances of different types causes class cast exception
c29681a
to
80c8862
Compare
Suggested splits:
What do you think? |
How about the third - i.e. "Support intersection"? |
I've issued a new PR up to "Support intersection" #100 |
In this PR, a number of time series transformations are (would be) contributed.
Those kind of transformations are basically transformations of the underlying date time index such that:
new_ts = ts.transform
is equivalent tonew_ts = ts.rebase(ts.index.transform)
new_ts = transform(ts1, ts2)
is equivalent to