-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Initial work to add a Time type to Base.Dates #12274
Conversation
[CI cancelled because it's not worth running at this point] |
(+)(x::Time,y::Dates.TimePeriod) = return Time(NS(value(x)+Dates.tons(y))) | ||
(-)(x::Time,y::Dates.TimePeriod) = return Time(NS(value(x)-Dates.tons(y))) | ||
(+)(y::Dates.TimePeriod,x::Time) = x + y | ||
(-)(y::Dates.TimePeriod,x::Time) = x - y |
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.
Do you intend 2 hours - 12:30
to be 10:30
?
Jacob: thanks a
Regarding printing/parsing... I would personally like to see methods like:
Your string/print is very similar, but I don't think I saw a string-based constructor. Of course, once this becomes part of Base, we could specialize some formatting options in the Formatting.jl package. Thanks again for the effort! |
postfixes for subsecond units
|
As time of day is frequently 'flattened' into seconds or subseconds of a day and the preferred subsecond resolution varies with the application aor the computational environment, one approach to processing a time of day to allow parametric content or to offer a defaulted argument or to let the content of the string or of the tuple (and better, a prioritized mix of, say, two of those) to select working subsecond resolution. |
Bump. Would be nice to get this one done. |
Just a few notes and a question: Time must use an integer underlying type, floats don't work well A week is the largest amount of time in common time units that is Given the state of the art in ultra-high frequency physics, where What is of greater import, keeping the storage size to 64 bits |
Int128 is not very well-supported on all platforms or compilers. |
that is a shame, and a deciding factor. On Tue, Nov 24, 2015 at 1:58 PM, Tony Kelman notifications@github.com
|
I am not the guy to fold Time in with Date; however if something along #=
=# immutable RelativeTime immutable GivenTime Times = Union{RelativeTime, GivenTime} Yoctoseconds{T<:Times}(t::T) = t.y function dSeconds(t::Float64) nSeconds(t::T) = GivenTime(idiv(t,Int64(1e6)),0) On Tue, Nov 24, 2015 at 2:09 PM, Jeffrey Sarnoff jeffrey.sarnoff@gmail.com
|
What's the plan here? This has been pretty quiet and it probably isn't 0.5 material unless it gets finished up very soon. |
I took the non-response as preference for another approach -- I still am not the guy to merge Time into Date (I have not used Date and defer to @quinnj on its construction and what best works to interpose Time within that extensive software subfacility). OTOH, if we want to -- I'd spend the weekend doing some coding to have a Type of Time (and Type of Span of that Type of Time) that understands weeks, days, hours, minutes, seconds, microseconds, milliseconds, nanoseconds. I'd give it correct arithmetic. |
@tkelman Is Int128 better supported with v0.5? |
I think there's a couple different ideas here:
Those might each be better as separate PRs. It's not really clear which one each interested party is after. 1) is probably easy to implement using the work here and in the issue. 2) might need more consideration. |
I think we should focus on use-case 1). The second case is much better served by going the SIUnits.jl route, where you already get a parametric I'm happy to push this through in the next few days if people really want it. I also think this would fit well in a package; are there strong feelings one way or the other at this point? (package vs. in Base) |
Hi Jacob, |
I think the performance issues that are just now being identified by Simon and others in the Dates code are indicative of just how long it can take to get these things right, and we probably shouldn't rush new features if you can see a good way to separate concerns with Dates in Base and Time types in a package. Some of the Dates internals might also be up for some refactoring to address those performance issues, which seem to have more people working on them right now with a higher priority, so I'd focus there at the moment. |
@Jeffrey-Sarnoff Int128 probably works a bit better with the more recent LLVM versions used on master, but it could always use more testing. If we ever look more seriously at supporting a Visual-Studio built version of Julia, there may still be gaps in Int128 support there. |
Don't know if this is the right issue to comment on this, but from a quant/trader's perspective, having the ability to merge high frequency tick data with nanosecond precision time-stamps is becoming a big deal in the industry. R's xts library does not currently support this. I believe pandas does. I for one would love to see this functionality enabled sooner than later. |
Ok, made some progress here and this is actually getting close in terms of the rest of the infrastructure we have in Dates. (plus lots of tests, yay!) Would definitely love some review/feedback. One of the biggest design questions I ran into was the following:
I think I'm leaning towards treating |
I think
|
Something I didn't see in the PR is |
I don't think there's anything left to do here though; I was just waiting on the datetime-parsing PR to merge before rebasing and merging this. I can rebase and merge in the next few hours. |
@@ -184,6 +191,31 @@ function DateTime(func::Function, y, m, d, h, mi, s; step::Period=Millisecond(1) | |||
return adjust(DateFunction(func, negate, DateTime(y)), DateTime(y, m, d, h, mi, s), step, limit) | |||
end | |||
|
|||
""" | |||
Time(f::Function, h[, mi, s, ms, us]; step=Second(1), negate=false, limit=10000) -> Time |
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.
do you want these docstrings to be in the stdlib docs or repl-only?
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.
the default step isn't always one second in the implementations below
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.
Should the docstrings be in stdlib or repl-only? I don't have a preference either way, but if there's a convention we should follow, happy to do it. Yeah, I would hope it would be obvious that if you're doing sub-Second Time ranges, it wouldn't default to a Second (since that would potentially lose precision in range steps).
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.
If the Dates section of the stdlib docs includes other docstrings for non-exported types, then this should be added.
Docstring saying one thing and code doing another isn't obvious.
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.
If the Dates section of the stdlib docs includes other docstrings for non-exported types, then this should be added.
So does it?
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 added Time docs to the stdlib.
(+)(x::Time, y::TimePeriod) = return Time(Nanosecond(value(x) + tons(y))) | ||
(-)(x::Time, y::TimePeriod) = return Time(Nanosecond(value(x) - tons(y))) | ||
(+)(y::Period, x::TimeType) = x + y | ||
(-)(y::Period, x::TimeType) = x - y |
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.
are these magnitudes then?
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.
Not sure I understand your comment. These are generic fallbacks that will end up dispatching to the definitions above. This is just for the switched order of operators.
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.
It looks wrong that y - x = x - y
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.
Ah, got it. Yes, these are magnitudes. Order doesn't matter.
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.
worth a comment then, in case anyone else gets confused looking at this
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.
Because we already do
julia> Dates.Day(1) - Date(2016, 1, 1)
2015-12-31
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.
wat? why allow that?
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 don't remember if it was an explicit decision to allow or not. We can open another issue to remove.
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'm in favor of removing this.
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.
opened #20205
reference = period == :Week ? " For details see [`$accessor_str(::$typ_str)`](@ref)." : "" | ||
typs = period in (:Microsecond, :Nanosecond) ? ["Time"] : | ||
period in (:Hour, :Minute, :Second, :Millisecond) ? ["Time", "DateTime"] : ["Date","DateTime"] | ||
reference = period == :Week ? " For details see [`$accessor_str(::Union{Date,DateTime})`](:func:`$accessor_str`)." : "" |
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.
old-style cross-reference format
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.
What's the new style?
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.
(@ref)
, the way the line used to be
$($name)(t::Time) -> Int64 | ||
|
||
The $($name) of a `Time` as an `Int64`. | ||
""" $func(t::Time) |
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.
spacing is off
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.
How so? This follows the spacing of the block directly above it....
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.
L134: 4 spaces
L135: 4 spaces
L136: 8 spaces
L137: 12 spaces
L138: NA
L139: 9 spaces <- should be 8
L140: 9 spaces <- should be 8
L141: 4 spaces
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.
Ah, thanks. Sorry for missing this. Fixed.
Docs updated. |
throwing an error (in the case that `f::Function` is never satisfied). | ||
throwing an error (in the case that `f::Function` is never satisfied). Note that the default step | ||
will adjust to allow for greater precision for the given arguments; i.e. if hour, minute, and second | ||
arguments are provided, the default step will be `Millisecond(1)` instead of `Second(1)`. |
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'd either leave out the default in the docstring signature and say something like "the default value of step
is the smaller of Second(1)
or 1 unit of the next smaller precision than provided as input" in the description, or probably clearer, just write out the different signatures since square brackets for optional args isn't Julia syntax anyway
Time(f::Function, h, mi=0; step=Second(1), negate=false, limit=10000) -> Time
Time(f::Function, h, mi, s; step=Millisecond(1), negate=false, limit=10000) -> Time
Time(f::Function, h, mi, s, ms; step=Microsecond(1), negate=false, limit=10000) -> Time
Time(f::Function, h, mi, s, ms, us; step=Nanosecond(1), negate=false, limit=10000) -> Time
(edited to fix Microsecond typo)
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.
was not addressed, docstring should not list a default value that isn't correct
@test a in dr | ||
|
||
@test all(x->sort(x) == (step(x) < zero(step(x)) ? reverse(x) : x),drs) | ||
@test all(x->step(x) < zero(step(x)) ? issorted(reverse(x)) : issorted(x),drs) |
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.
space after the comma in these all
and map
calls would look quite a bit better
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.
was not addressed
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.
Unfortunately, the entire dates module/files have inconsistent spacing. I can do a separate PR to do a bunch of these spacing issues all at once.
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.
Though I'll probably wait until after the datetime-parsing PR so they don't have to rebase as much.
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.
newly added lines aren't going to conflict with the other PR if it won't need to touch them
@@ -168,6 +192,12 @@ ms = Dates.Millisecond(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) | |||
|
|||
@test isfinite(Dates.Date) | |||
@test isfinite(Dates.DateTime) |
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.
these isfinite
lines probably shouldn't be deleted
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.
Added them back in.
|
Correct me if I'm wrong, but that never happened? What changed? |
At the time, I thought it'd be more useful to have a |
They're all addressed in #20226 |
Addresses #12140.
TODO:
show
) micro/nanoseconds