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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jan 24, 2017
Merged

Initial work to add a Time type to Base.Dates #12274

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b8b33f9
Add a Time type to the Base.Dates module
quinnj Jan 5, 2017
73eafe9
Time cleanup
quinnj Jan 6, 2017
58338da
Fix Time doc
quinnj Jan 19, 2017
8881760
Docs cleanup
quinnj Jan 19, 2017
673d2b8
Fix whitespace
quinnj Jan 19, 2017
e383f20
Fix Time docs
quinnj Jan 23, 2017
6cd4ea5
Add isfinite tests for TimeTypes
quinnj Jan 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Docs cleanup
  • Loading branch information
@quinnj
quinnj committed Jan 19, 2017
commit 88817600dcede9567a658054e34ca40b504b04ff
4 changes: 3 additions & 1 deletion base/dates/adjusters.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,9 @@ provided `h, mi, s, ms, us` arguments, and will be adjusted until `f::Function`
size in adjusting can be provided manually through the `step` keyword. If `negate=true`,
then the adjusting will stop when `f::Function` returns `false` instead of `true`. `limit`
provides a limit to the max number of iterations the adjustment API will pursue before
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)`.
Copy link
Contributor

@tkelman tkelman Jan 20, 2017 •
edited
Loading

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)

Copy link
Contributor

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

"""
Time(::Function, args...)

Expand Down
2 changes: 1 addition & 1 deletion base/dates/periods.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ for period in (:Year, :Month, :Week, :Day, :Hour, :Minute, :Second, :Millisecond
# Period accessors
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`)." : ""
reference = period == :Week ? " For details see [`$accessor_str(::Union{Date, DateTime})`](@ref)." : ""
for typ_str in typs
@eval begin
@doc """
Expand Down
9 changes: 9 additions & 0 deletions doc/src/stdlib/dates.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ Base.Dates.UTInstant
Base.Dates.TimeType
Base.Dates.DateTime
Base.Dates.Date
Base.Dates.Time
```

## Dates Functions
Expand All @@ -35,6 +36,10 @@ Base.Dates.Date(::Function, ::Any, ::Any, ::Any)
Base.Dates.Date(::Base.Dates.TimeType)
Base.Dates.Date(::AbstractString, ::AbstractString)
Base.Dates.Date(::AbstractString, ::Base.Dates.DateFormat)
Base.Dates.Time(::Int64::Int64, ::Int64, ::Int64, ::Int64, ::Int64)
Base.Dates.Time(::Base.Dates.Period...)
Base.Dates.Time(::Function, ::Any...)
Base.Dates.Time(::Base.Dates.TimeType)
Base.Dates.now()
Base.Dates.now(::Type{Base.Dates.UTC})
Base.eps
Expand All @@ -51,6 +56,8 @@ Base.Dates.hour
Base.Dates.minute
Base.Dates.second
Base.Dates.millisecond
Base.Dates.microsecond
Base.Dates.nanosecond
Base.Dates.Year(::Base.Dates.TimeType)
Base.Dates.Month(::Base.Dates.TimeType)
Base.Dates.Week(::Base.Dates.TimeType)
Expand All @@ -59,6 +66,8 @@ Base.Dates.Hour(::DateTime)
Base.Dates.Minute(::DateTime)
Base.Dates.Second(::DateTime)
Base.Dates.Millisecond(::DateTime)
Base.Dates.Microsecond(::Time)
Base.Dates.Nanosecond(::Time)
Base.Dates.yearmonth
Base.Dates.monthday
Base.Dates.yearmonthday
Expand Down
2 changes: 1 addition & 1 deletion test/dates/accessors.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ end

# test hour, minute, second
let h=0, mi=0, s=0, ms=0, us=0, ns=0
for h = 0:23, mi = 0:59, s = 0:59,
for h = (0,23), mi = (0,59), s = (0,59),
ms in (0,1,500,999), us in (0,1,500,999), ns in (0,1,500,999)
Copy link
Member

Choose a reason for hiding this comment

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

@quinnj you can mitigate the segfault by reducing how many iterations this loop has. For example changing s = 0:59 to s = (0,59) seems to avoid the segfault on 32-bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

that seems like a deeper bug...

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. I'll take another look into this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any more understanding of the cause?

Copy link
Member

Choose a reason for hiding this comment

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

The segfault seems unrelated to this PR. I'm attempting a bisect to identify the root cause.

Copy link
Contributor

Choose a reason for hiding this comment

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

what's the test case look like, just for reference?

Copy link
Member

Choose a reason for hiding this comment

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

The new accessor test case triggers the GC segfault. From what I've seen so far it seems that 32-bit systems have this issue when they perform a large amount of tests. Since this particular test generates 33177600 @test calls it appears to regularly trigger the problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the info. It will be v,good solution --the 64 bit GC wants the same adjustment.

Knowing that nothing in this PR gives rise to the GC failure by its manner of work, rather the numerosity of that test has brought to our awareness that deeper bug. Do we feel it helpful to allow this (with the lesserly agglomerative test replacing the new accessor test) to merge. I think it important that people have some time with this Time before v0.6 rolls into home and hearth.

Any few brief wants of the dev community would make it all the nicer for the rest of us>

Copy link
Member Author

Choose a reason for hiding this comment

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

@shashi, @omus, I'd say let's go ahead and merge the date-parsing PR first (I think there's just a tweak or two left there), and then I'll do a rebase here and we can merge. @omus, if we can put together a minimum repro of the 32-bit segfault, we can open a separate issue with that and reduce the test case here so it doesn't fail.

Copy link
Member

Choose a reason for hiding this comment

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

Finally created the issue: #20107

t = Dates.Time(h, mi, s, ms, us, ns)
@test h == Dates.hour(t)
Expand Down