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

Add Dates module to Base #7654

Merged
merged 30 commits into from
Aug 16, 2014
Merged

Add Dates module to Base #7654

merged 30 commits into from
Aug 16, 2014

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Jul 18, 2014

Fixes #3524.

Cross-reference: #5328, quinnj/Dates.jl#6, quinnj/Datetime.jl#12.

Successor to #7285 PR because of a weird github merge bug.

There were a few tweaks to the code from the original PR, nothing major. Additional tests and docs! I'd especially appreciate feedback on the content and structure of the docs.

@JeffBezanson
Copy link
Member

The docs and tests for this are amazing! Very impressive.

@quinnj
Copy link
Member Author

quinnj commented Jul 24, 2014

As was brought up here, there's currently an inconsistency in how inputs are checked/validated when building Dates/DateTimes by parts. Currently, only months are checked that they are in the range 1..12, otherwise an error is thrown. For other parts (day, hour, minute, etc.), values outside a valid range serve to "roll" back or forward the date. So Date(2014,10,32) would result in 2014-11-01, etc.

I'd love to get some feedback on what people think we should do here. Here are the options/arguments:

Validate that all inputs are sane:

  • Pros: No subtle errors if you happen to pass a wrong value by accident, you'll see an error very quickly.
  • Cons: Slower because we have to validate everything. Not that much slower for Dates, but DateTime construction has a noticeable hit. The other con here I've noticed is it makes it quite a bit more finicky to work with databases because db's usually allow any kind of date and use something like -9999-00-00 as a null date of sorts. The pain is in trying to retrieve db dates directly into julia Date and errors are thrown everywhere.
  • Other languages that do this: Java 8, Python

Option 2, don't validate any input, but extend the "roll" forward/backward behavior to months:

  • Pros: Really fast, clean implementation. As noted above, allows for interfacing with "bad dates" in a way that doesn't barf on construction
  • Cons: Can result in subtle bugs if a user accidently starts passing values out of range and is surprised to either not get an error or with the results.
  • Other languages that do this: Go, Javascript

I feel I could go either way on this, but honestly, I don't think it's a very common case that you accidentally pass an out of range value and it comes back to bite you if it was rolled forward. I think with clear documentation, this could even be a feature in terms of certain algorithms or calculations and as noted above in working with certain database values.

I'd love to hear everybody's thoughts though.

@nalimilan
Copy link
Member

Tough call. I think I would prefer to get the checks by default, with an option to disable them to get the rollback behavior when needed. It would be interesting to know whether raising an error with incorrect dates creates issues in certain cases in Java 8 and Python.

@quinnj
Copy link
Member Author

quinnj commented Jul 29, 2014

Thanks for the input @nalimilan. I spent a good chunk of time coding up both scenarios yesterday and comparing performance, etc and I think it'll be best to go with the checks by default. It really doesn't add any hit to Dates, but DateTimes are just under 2x slower, which isn't too bad in my book for having consistency. I'm still open to other ideas if anyone has them, but I think we'll go with the checks by default for now.

…ethods. Also give better typed results with vectorized arithmetic operations
…rect by using a single inner 'len' function that is used in length(), steprem(), and in(). Also added lots and lots of tests to try to cover all combinations of Date/DateTime, different Period steps, with positive and negative step values.
@quinnj
Copy link
Member Author

quinnj commented Aug 8, 2014

Code for ranges is such a beast. I'm probably a little overboard on the range tests now, but there are so many cases to check for. On the bright side, the date range code is now way cleaner (and more correct!).

@quinnj
Copy link
Member Author

quinnj commented Aug 11, 2014

I'd like to propose this go into 0.4. At this point, the code has been through several iterations and most parts seem to have settled as a good API. I'd still appreciate getting a few heads together for a hangout/irc code review to sanity check everything, but I think getting it into people's hands at this point will help shake anything else out. Any volunteers for a hangout?

Fun fact: the current breakdown of lines is: 1000 lines of code, 750 lines of docs, and 2500 lines of tests.

@StefanKarpinski
Copy link
Member

+1 for this in 0.4.

@quinnj
Copy link
Member Author

quinnj commented Aug 16, 2014

Guys, I just realized that today is August 16, which is a date, and this is a PR all about dates, so what a better reason to merge! The coincidence is incredible!

@johnmyleswhite
Copy link
Member

Can't fault that kind of argument.

@JeffBezanson
Copy link
Member

I say merge it. I'm excited. Who gets to push the button?

ivarne added a commit that referenced this pull request Aug 16, 2014
@ivarne ivarne merged commit 230c030 into master Aug 16, 2014
@ivarne
Copy link
Member

ivarne commented Aug 16, 2014

I got to push the button :-)

@JeffBezanson
Copy link
Member

LOL

@StefanKarpinski
Copy link
Member

Hilarious.

@ivarne
Copy link
Member

ivarne commented Aug 16, 2014

Or maybe I should have waited for @quinnj, I feel kind of bad.

@StefanKarpinski
Copy link
Member

Eh, it's done now. If everything breaks it's your fault :-P

I'm excitedly building the new master with built-in Dates support. Even though I've tried the branch before, somehow it feels different once it's actually merged. Like now it's "real".

@aviks
Copy link
Member

aviks commented Aug 16, 2014

Woo hoo!

I'll get on to porting my packages. Maintaining backwards compatibility will be interesting. There are some packages that depend on nolta/Calendar.jl. Some depend on quinj/Datetime.jl.

@quinnj : Will you maintain Dates.jl with a compatible API for the 0.3 - 0.4 cycle? That might actualy be the best option for packages. Packages can then conditionally import Dates.jl, and subsequently work with a single API. It'll be a shame if packages become "0.4 only" due to using Base.Dates.

@tkelman
Copy link
Contributor

tkelman commented Aug 16, 2014

How long is the dates test supposed to take? On my RHEL5 system it appears to be hanging

@tkelman
Copy link
Contributor

tkelman commented Aug 16, 2014

Hm nevermind, either I'm impatient or it's intermittent. The date tests do take about a minute on Linux, it would be good to move them earlier in the list for the sake of parallel testing.

@quinnj
Copy link
Member Author

quinnj commented Aug 16, 2014

Awesome. Just awesome. No worries on the merge @ivarne; I probably should have realized I would be driving for a good portion of the evening. Just happy it's in!

@aviks, yeah, I can keep the Dates.jl package going for the 0.4 cycle, if packages want to use it as a stepping stone for 0.3 support. The bigger issue is getting packages over from Datetime.jl; while the API isn't hugely different, if they were relying heavily on timezone support, that's the main difference now with functionality split out into TimeZones.jl.

@tkelman, there has been a little weirdness with travis lately, see @IainNZ's comments on #7787. I don't think I've ever seen anything locally though. I could probably trim down some of the tests as some of the tests take an input for how many iterations to run. I agree though that it would probably be good to move it earlier in the testing process.

@tkelman
Copy link
Contributor

tkelman commented Aug 16, 2014

Not talking about Travis, rather locally. But on both I see some warnings about imported bindings for oct, dec, e overwritten in base, you might want some let statements in your tests for local variables?

@quinnj
Copy link
Member Author

quinnj commented Aug 16, 2014

That's a good point. I wonder why I've never seen those before in all my testing?

@IainNZ
Copy link
Member

IainNZ commented Aug 17, 2014

This is a nonbreaking merge right?
(don't forget NEWS.md)

@quinnj
Copy link
Member Author

quinnj commented Aug 17, 2014

Non-breaking was definitely the plan, unless we see some kind of weirdness with Base.Dates vs. Dates.jl out there. That's an interesting question actually, if I have the Dates.jl package installed and run using Dates, will it pull the package in or the Base module in now? I would imagine Base has preference here, right?

Good call @IainNZ on the news.md, I'll do a little writeup.

@IainNZ
Copy link
Member

IainNZ commented Aug 17, 2014

We'll find out if its breaking tomorrow morning, I suppose :)

@quinnj
Copy link
Member Author

quinnj commented Aug 17, 2014

Hmmm.......why don't Date, DateTime, or now seem to be available in Main? Did I fudge the exporting somehow? Do we need to import those three methods in sysimg.jl?

@tkelman
Copy link
Contributor

tkelman commented Aug 17, 2014

Dates test is definitely freezing occasionally on Win32 and 64-bit Ubuntu 14.04.

@quinnj
Copy link
Member Author

quinnj commented Aug 17, 2014

Ok, I think I fixed the exports in sysimg.jl with this commit.

Thanks for the report @tkelman, it would be great if you could narrow down which test file (in the test/dates directory) was hanging. I'll also fire up a 32-bit and see what I can find.

@tkelman
Copy link
Contributor

tkelman commented Aug 17, 2014

@quinnj as best as I can tell with some printlns it's getting stuck the third time through this section

dr = f:pos_step:l
len = length(dr)
@test len > 0
@test typeof(len) <: Int
@test !isempty(dr)
@test first(dr) == f
@test last(dr) <= l
@test minimum(dr) == first(dr)
@test maximum(dr) == last(dr)
@test dr[1] == f
@test dr[end] <= l
@test next(dr,start(dr)) == (first(dr),1)
on Win32

Another Win32 freeze happened somewhere in

julia/test/dates/types.jl

Lines 148 to 172 in 4ba73cf

y = Dates.Year(1)
m = Dates.Month(1)
w = Dates.Week(1)
d = Dates.Day(1)
h = Dates.Hour(1)
mi = Dates.Minute(1)
s = Dates.Second(1)
ms = Dates.Millisecond(1)
@test Dates.DateTime(y) == Dates.DateTime(1)
@test Dates.DateTime(y,m) == Dates.DateTime(1,1)
@test Dates.DateTime(y,m,d) == Dates.DateTime(1,1,1)
@test Dates.DateTime(y,m,d,h) == Dates.DateTime(1,1,1,1)
@test Dates.DateTime(y,m,d,h,mi) == Dates.DateTime(1,1,1,1,1)
@test Dates.DateTime(y,m,d,h,mi,s) == Dates.DateTime(1,1,1,1,1,1)
@test Dates.DateTime(y,m,d,h,mi,s,ms) == Dates.DateTime(1,1,1,1,1,1,1)
@test Dates.DateTime(Dates.Day(10),Dates.Month(2),y) == Dates.DateTime(1,2,10)
@test Dates.DateTime(Dates.Second(10),Dates.Month(2),y,Dates.Hour(4)) == Dates.DateTime(1,2,1,4,0,10)
@test Dates.DateTime(Dates.Year(1),Dates.Month(2),Dates.Day(1),Dates.Hour(4),Dates.Second(10)) == Dates.DateTime(1,2,1,4,0,10)
@test Dates.Date(y) == Dates.Date(1)
@test Dates.Date(y,m) == Dates.Date(1,1)
@test Dates.Date(y,m,d) == Dates.Date(1,1,1)
@test Dates.Date(m) == Dates.Date(1,1,1)
@test Dates.Date(d,y) == Dates.Date(1,1,1)
@test Dates.Date(d,m) == Dates.Date(1,1,1)
@test Dates.Date(m,y) == Dates.Date(1,1,1)

These are intermittent and might be Julia's fault instead of anything specific to dates, they look a lot like #7942 which previously wasn't happening on Win32.

@quinnj
Copy link
Member Author

quinnj commented Aug 17, 2014

Yeah, I think I've got that one figured out, but I'm running into a weird no method bug with similar further down in the tests.

@wlbksy
Copy link
Contributor

wlbksy commented Aug 17, 2014

I think there should be an entry both in doc/manual/index.rst and doc/index.rst

@IainNZ
Copy link
Member

IainNZ commented Aug 17, 2014

Only problem this seems to have maybe triggered: JuliaFinMetriX/TimeData.jl#1

@quinnj quinnj deleted the jq/dates branch August 17, 2014 21:50
@quinnj
Copy link
Member Author

quinnj commented Aug 18, 2014

Ok @tkelman, I fixed the 32-bit issues I've seen locally in 9f81a60 and e3e86b5. Note that the range tests using map are still failing on 32-bit due to #7709, but otherwise, everything should be working well. Please let me know if you see any other 32-bit or linux failures. Hopefully we can track down any other problems.

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.

Time series support in Base?