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

Jump to bottom

Support TimeSpan construction from Dates.CompoundPeriod, Dates.Period #57

Merged
merged 6 commits into from
May 23, 2023
Merged

Support TimeSpan construction from Dates.CompoundPeriod, Dates.Period #57

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e79c0bf
support construction from periods
hannahilea May 23, 2023
ce470c2
bump version
hannahilea May 23, 2023
4f7139a
support different start, stop types
hannahilea May 23, 2023
6c071a7
use compat and dates.convert
hannahilea May 23, 2023
3e3a834
more tests
hannahilea May 23, 2023
a5ae271
make inner constructor outer constructor
hannahilea May 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
name = "TimeSpans"
uuid = "bb34ddd2-327f-4c4a-bfb0-c98fc494ece1"
authors = ["Beacon Biosignals, Inc."]
version = "0.3.7"
version = "0.3.8"

[deps]
Compat = "34da2185-b29b-5c13-b0c7-acf172513d20"
Dates = "ade2ca70-3891-5945-98fb-dc099432e06a"
Statistics = "10745b16-79ce-11e8-11f9-7d13ad32a3b2"

[compat]
Compat = "3.38,4"
julia = "1.6"

[extras]
Expand Down
6 changes: 5 additions & 1 deletion src/TimeSpans.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
module TimeSpans

using Base.Iterators
using Compat
using Dates
using Statistics

Expand Down Expand Up @@ -34,7 +35,10 @@ struct TimeSpan
start < stop || throw(ArgumentError("start(span) < stop(span) must be true, got $start and $stop"))
return new(start, stop)
end
TimeSpan(start, stop) = TimeSpan(Nanosecond(start), Nanosecond(stop))
function TimeSpan(start, stop)
_to_ns = (t) -> isa(t, Dates.CompoundPeriod) ? convert(Nanosecond, t) : Nanosecond(t)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should just always convert? That is the default behavior when you don't have an inner constructor, for example;

julia> struct A
       n::Nanosecond
       end
julia> A(1)
ERROR: MethodError: Cannot `convert` an object of type Int64 to an object of type Nanosecond
Closest candidates are:
  convert(::Type{T}, ::Dates.CompoundPeriod) where T<:Period at ~/.julia/juliaup/julia-1.8.5+0.x64.linux.gnu/share/julia/stdlib/v1.8/Dates/src/periods.jl:363
  convert(::Type{Nanosecond}, ::Microsecond) at ~/.julia/juliaup/julia-1.8.5+0.x64.linux.gnu/share/julia/stdlib/v1.8/Dates/src/periods.jl:433
  convert(::Type{Nanosecond}, ::Millisecond) at ~/.julia/juliaup/julia-1.8.5+0.x64.linux.gnu/share/julia/stdlib/v1.8/Dates/src/periods.jl:433
  ...
Stacktrace:
 [1] A(n::Int64)
   @ Main ./REPL[7]:2
 [2] top-level scope
   @ REPL[11]:1

You can see in the stacktrace that it tries to call convert. But if you have an inner constructor, it calls that and doesn't have the default constructor w/ convert:

julia> struct B
       n::Nanosecond
       B(n::Nanosecond) = new(n)
       end

julia> B(1)
ERROR: MethodError: no method matching B(::Int64)
Closest candidates are:
  B(::Nanosecond) at REPL[9]:3
Stacktrace:
 [1] top-level scope
   @ REPL[10]:1

I believe we have the inner constructor to do the start == stop case, but I don't think we were aiming to get rid of the convert constructors. So I think just auto-converting when we don't hit the ::Nanosecond one might be the best approach:

Suggested change
_to_ns = (t) -> isa(t, Dates.CompoundPeriod) ? convert(Nanosecond, t) : Nanosecond(t)
_to_ns = convert(Nanosecond, t)

Note that convert(Nanosecond, Nanosecond(1)) works, so even if start::CompoundPeriod and stop::Nanosecond, the extra convert should be harmless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should! we weren't b/c I thought that Alex's "Compat" fix wasn't working, because I didn't implement it correctly :lol-cry:. BUT the most recent implementation does what you're suggesting, I think---take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh hang on! let me reread

Copy link
Member

Choose a reason for hiding this comment

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

ah oops, yeah, agree w/ the current implementation!

I was suggesting something different (always convert) but I missed that we already had

TimeSpan(start, stop) = TimeSpan(Nanosecond(start), Nanosecond(stop))

in the code base, in which case using the constructor (Nanosecond()) instead of convert makes sense as the fallback. Ok agreed with what we have!

return TimeSpan(_to_ns(start), _to_ns(stop))
end
end

"""
Expand Down
22 changes: 20 additions & 2 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
using Test, TimeSpans, Dates

using Test
using TimeSpans
using TimeSpans: contains, nanoseconds_per_sample
using Compat
using Dates
using Statistics

function naive_index_from_time(sample_rate, sample_time)
Expand Down Expand Up @@ -45,6 +47,22 @@ end
@test translate(t, by) === TimeSpan(start(t) + Nanosecond(by), stop(t) + Nanosecond(by))
@test translate(t, -by) === TimeSpan(start(t) - Nanosecond(by), stop(t) - Nanosecond(by))
@test repr(TimeSpan(6149872364198, 123412345678910)) == "TimeSpan(01:42:29.872364198, 34:16:52.345678910)"

# Periods and compound periods are supported
for start in [Nanosecond(3), Minute(1), Minute(3) + Nanosecond(1)]
stop = start + Nanosecond(8)
start_ns = convert(Nanosecond, start)
stop_ns = convert(Nanosecond, stop)
@test TimeSpan(start, stop) == TimeSpan(start_ns, stop_ns) == TimeSpan(Dates.value(start_ns), Dates.value(stop_ns))
end
@test_throws MethodError TimeSpan(now(), now() + Nanosecond(1))

# Different types for start and stop are supported
for (start, stop) in [(3, Nanosecond(8)), (Nanosecond(3), 8), (3, Minute(8))]
start_ns = Nanosecond(start)
stop_ns = Nanosecond(stop)
@test TimeSpan(start, stop) == TimeSpan(start_ns, stop_ns) == TimeSpan(Dates.value(start_ns), Dates.value(stop_ns))
end
end

@testset "format_duration" begin
Expand Down