Skip to content
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

WIP: Adding TimeType type parameter #211

Closed
wants to merge 5 commits into from
Closed

WIP: Adding TimeType type parameter #211

wants to merge 5 commits into from

Conversation

GordStephen
Copy link
Contributor

As discussed in #207, this would add a type parameter to TimeArray describing the type of the timestamps being used.

One question raised by the failing tests is, what to do with index lookups when the timestamp index passed in isn't the same type as the timestamps? Historically Date and DateTime have been used a bit interchangeably, but if we want to support other theoretical TimeTypes this may have to be revisited...

Also, the tests (for both master and this branch) started failing on me all of a sudden without using TimeSeries and Base.Dates in runtests.jl. I'm a bit puzzled because Travis didn't seem to have a problem with the same commits, and as far as I could tell was running the same versions of Julia, FactCheck, etc... Not sure what was up there, but I added in the necessary lines here.

@GordStephen
Copy link
Contributor Author

@milktrader are you OK if we restrict lookups to the TimeType of the array timestamps? Right now it's possible to acess a Date-indexed row with a DateTime object, for example.

@GordStephen
Copy link
Contributor Author

Changed to/from/collapse to be TimeType-agnostic. Tests currently fail due to breaking changes this PR would cause:

1 - Loss of ability to lookup Date-indexed rows by a DateTime object
2 - to and from methods change from:

to{T,N}(ta::TimeArray{T,N}, y::Int, m::Int, d::Int)
to{T,N,D}(ta::TimeArray{T,N,D}, d::D)

If everyone's ok with these, we can add deprecation warnings to the next release and then make the changes later on, and I'll update the tests here accordingly.

@milktrader
Copy link
Contributor

Sorry for being out the last couple weeks, Ironman is a time sink if you ever decide to do one (pro tip).

I have to think about this PR a little.

@milktrader
Copy link
Contributor

Ok, I scanned over the code and put down some notes. Overall, I like this idea. Since this is a significant change, we'll probably want to go to version 0.7.0 with these changes. (this would be something to add to the NOTES.md).

You mentioned

Changed to/from/collapse to be TimeType-agnostic. Tests currently fail due to breaking changes this >PR would cause:

1 - Loss of ability to lookup Date-indexed rows by a DateTime object
2 - to and from methods change from:

How about his approach ... you complete the transition to TimeType and push it as a branch. Call it 0.7.0, for example. We can then experiment with it some more there and merge it with master after running it through some more paces.

Version 0.6.x is fairly stable now and I expect that even with some minor changes to it, merging this big change will only create a manageable amount of merge conflicts that should be straightforward to correct.

@GordStephen
Copy link
Contributor Author

Ok, sounds good. We should probably tag at least one more 0.6.x version that adds warnings to methods that 0.7.0 would deprecate (current to/from methods, etc).

@milktrader
Copy link
Contributor

Go ahead and merge and tag a new 0.6.x version (:patch) and then we can stabilize the 0.6.x (master) branch for work on the 0.7.0 branch.

@GordStephen
Copy link
Contributor Author

In the process of being implemented in #223

@GordStephen GordStephen deleted the timetype branch October 26, 2015 23:16
Sign up for free to 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