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: Modified Date types to allow use of Float for definitive periods. #9424

Closed
wants to merge 1 commit into from

Conversation

kbuzby
Copy link

@kbuzby kbuzby commented Dec 20, 2014

modified Types such that Year and Month are part of CalendarPeriod wrapp...ed around Int
Week, Day, Hour, Minute, Second are wrapped around Real to allow for both Int and Float types
Millisecond remained wrapped around Int
Ref Issue #9393

@eval immutable $T <: TimePeriod
value::Int64
$T(v::Number) = new(v)
value::Real
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to make these parametric so the value field can be concretely-typed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should just be Float64. I think making it parametric can come later with concrete use cases.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a bunch of issues with making Week, Day, Hour, etc. strictly Float64. Specifically when testing ranges, which is why I ended up making these Real

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making the field type abstract is going to ruin performance, unfortunately.

@tkelman
Copy link
Contributor

tkelman commented Dec 21, 2014

Sorry about the initial unrelated failure on appveyor, but it does look like there is a legitimate failure here on 32-bit:

    From worker 2:       * dates
exception on 2: ERROR: test failed: (2013-01-01 == 2013-01-01)
 in expression: Dates.Date(2013) == test
 in error at error.jl:21
 in default_handler at test.jl:27
 in do_test at test.jl:50
 in include_string at loading.jl:97
 in include_from_node1 at no file
 in runtests at C:\projects\julia\test\testdefs.jl:5
 in anonymous at multi.jl:852
 in run_work_thunk at multi.jl:603
 in anonymous at task.jl:852
while loading C:\projects\julia\test\dates\types.jl, in expression starting on line 47
while loading dates.jl, in expression starting on line 6
ERROR: test failed: (2013-01-01 == 2013-01-01)
 in expression: Dates.Date(2013) == test
 in anonymous at task.jl:1616
while loading C:\projects\julia\test\dates\types.jl, in expression starting on line 47
while loading dates.jl, in expression starting on line 6
while loading C:\projects\julia\test\runtests.jl, in expression starting on line 42

@@ -1,3 +1,5 @@
using Base.Test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my apologies, missed this in cleaning up

@kbuzby kbuzby changed the title Modified Date types to allow use of Float for definitive periods. WIP: Modified Date types to allow use of Float for definitive periods. Dec 21, 2014
…apped around Int

Week, Day, Hour, Minute, Second are wrapped around Real to allow for conversion between
Millisecond remained wrapped around Int
Ref Issue JuliaLang#9393
@quinnj
Copy link
Member

quinnj commented Dec 23, 2014

Ok, here's a list of additional changes we'd need to consider/do:

  • Date and DateTime constructors currently convert all input arguments to Int64; with some Period types as Float64, we would have to do some kind of canonicalizing on inputs
  • It's also currently messy because Date is based on the internal type of Day; if Day is Float64, then we can technically have "partial" dates, i.e. 2014-01-01.5, which kind of defeats the purpose of having the Date type; FixedPeriod arithmetic has similar issues here
  • We would then probably need to restructure the Date type to still be based on some internal integer type to keep a day-precision TimeType
  • P(::AbstractString) where P<:Period needs to be updated to parse as a Float64 or Int64, depending on P
  • Similarly for typemax/typemin for Period types
  • Also the definitions of
($op){P<:Period}(x::P,y::Real) = P(($op)(value(x),Int64(y)))

since they depend on converting y to Int64

  • I don't quite understand all the implications, but @stevengj's recent work on canonicalizing CompoundPeriods might be affected as well; (e.g. Day(1.5) + Hour(12) == Day(2.0)?)
  • Similarly for the new Period conversion methods; these might be ok, but I'm not sure
  • I shudder to think of what this change would do to the dates range code (I'm not surprised you saw issues here); it seems like you'd have to use StepRanges for ranges involving Year or Month, but FloatRange for all other Period types; though it's a little weirder because it's more like you have integer start and stop with a potentially Float64 step value

So all in all, we'd probably have to restructure the Date type to be based on an internal integer type instead of a Float64 Day type. Then take care of the Period canonicalizing issues, then gird your loins for range hell.

Honestly, I really don't feel convinced yet that we need this change. Having walked through all the code considering how this affects everything, it just seems like it messes with some of the nice, simple, operations (dates internal representations, arithmetic, ranges, etc.), for not much gain.
I'd love to see some use-cases/examples where this is going to be worth the work and sacrifice of how simple things are currently.

@kbuzby
Copy link
Author

kbuzby commented Jan 6, 2015

@quinnj Have you had a chance to give any further thought to this issue? I've gotten a little distracted over the holidays myself, but looking to dive back in.

While I agree that Dates currently is very simple and nearly complete operationally, I would argue that it's best to minimize limitations in the language for sake of simplicity. The case which I would present is the desire to use specific units for a period without having to convert down. It's just as common for 18 Hours to be presented as 1.5 Days and converting between the two should not be impractical. It doesn't make sense to me that Day(Hour(18)) throws an InexactError when it is exactly 1.5 Days.

@nalimilan
Copy link
Member

"exactly 1.5 days". I love the quote. :-)

I'm not sure what your concrete use case is, but IMHO the strength of Dates is that they are forced to fit within the (very peculiar) framework of days, months and years. It's already complex enough. As @quinnj explained above, if non-integer values were allowed, then canonicalization of CompoundPeriod becomes even more complex (and you also introduce the issue of floating point approximation).

If you are working with hours, do not try to convert them in days, just use a function to display them in the most human-readable unit when you need to. What's the trouble with that solution?

@kbuzby
Copy link
Author

kbuzby commented Jan 6, 2015

I'd personally view that more as a workaround then a solution, but the issue would present itself when you are first working with ranges of days and begin to manipulate that data. The issue in which I first ran into this is in the testing of Gadfly. To create a bar graph with a range of Dates being the x-axis it looks for what the "span" of what one bar should be to create the graphic and subsequently the edge of the bar would be half the span width on either side of the actual point on the axis, and with the current implementation this is not possible because you can't have 0.5 days and converting to a more precise time period you are losing the convenience of being able to input a range in the units you intend the graph to show and have the graph show it as such.

@StefanKarpinski
Copy link
Member

For what it's worth, I'm sold on this functionality even though I'm not sure about the implementation.

@quinnj quinnj added the dates Dates, times, and the Dates stdlib module label Aug 18, 2015
@quinnj
Copy link
Member

quinnj commented Apr 28, 2016

I still stand by my previous comments here that the amount of complexity introduced by an internal Float64 representation are simply not worth the meager benefits. I'm still perfectly fine if someone wants to give this a shot and build out a full replacement; I'm happy to review, but I personally just don't see the benefits as being worth the cost here. (closing doesn't mean things can't still be pushed the the original branch here).

@quinnj quinnj closed this Apr 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dates Dates, times, and the Dates stdlib module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants