Given user-facing parameters that represent a time period actually units

[milankl]

At the moment we often have default parameters with second, minutes, hours, days scattered across the code. This basically requires the user to know whether output_dt=6 means hours, seconds, days, or whatever. Using Dates or Unitful might offer neat ways to solve this.

1. Dates

Say you have some user-facing type A which has a parameter time_scale. Then we could do

import Dates: Second, Minute, Hour, Day

Base.@kwdef struct A
     time_scale::Second = Hour(1)
end

So the time scale is by default 1 hour, note that the conversion happens automatically

julia> A()
A(Second(3600))

One can also provide other periods at creation

julia> A(time_scale=Day(2))
A(Second(172800))

Which is exactly what I want. So the user doesn't have to know the unit of time_scale nor needs to do a conversion manually (how many seconds are in 15 days???). However this would throw an error

julia> A(time_scale=1.0)
ERROR: MethodError: Cannot `convert` an object of type Float64 to an object of type Second

which we could circumvent with

function Base.convert(::Type{Second},x::Real)
    xr = round(Int64,x)
    @info "Rounding and converting $x to $xr for integer seconds."
    return convert(Second,xr)
end

function Base.convert(::Type{Second},x::Integer)
    @info "Input $x assumed to have units of seconds. Use Minute(x), Hour(x), Day(x) otherwise."
    return Second(round(Int64,x))
end

which yields very neatly a little info that what the assumed unit is when not providing it

julia> A(time_scale=11.9)
[ Info: Rounding and converting 11.9 to 12 for integer seconds.
[ Info: Input 12 assumed to have units of seconds. Use Minute(x), Hour(x), Day(x) otherwise.
A(Second(12))

julia> A(time_scale=12)
[ Info: Input 12 assumed to have units of seconds. Use Minute(x), Hour(x), Day(x) otherwise.
A(Second(12))

One (major?) drawback though is that only integer seconds are supported with Dates, sure we could use Millisecond etc but I'd really like to have internally everything as SI as possible to prevent a unitwar. @maximilian-gelbrecht @white-alistair what do you think?

[maximilian-gelbrecht]

Yes, I agree. When looking at #416 I also noticed again that currently time units are not mangaged that well. Mixing different units is confusing, doing everything with Dates is a good idea. I wasn't aware of this need automatic conversion. That makes this very useable.

[maximilian-gelbrecht]

That being said, when adjusting this to Dates we might encounter some minor issues due to rounding everything to full seconds as you already wrote. But I think that's not that bad.

[white-alistair]

The proposed solution here looks great and should be quite intuitive for new users. I'm not sure how an alternative Unitful solution would work but Dates has the enormous (imo) advantage that it's a stdlib.

I also can't immediately foresee major issues due to lack of sub-second resolution.

[maximilian-gelbrecht]

I was just implementing the behaviour for #416, and while doing so I stumbled over an (at least for me) unexpected behaviour that we need to watch out for when implementing this for the complete model.

So, first of all a lot of operations with floats are not defined for Seconds, etc. So at some point, one might need to do something like Dates.value(time_variable) * scaling_factor.

When using this in structs with @kwdef and default arguments this has the following interaction:

using Dates 

Base.@kwdef struct A 
    timeA::Second = Hour(1)
    timeB::Second = compute_time_B(timeA)
end 

compute_time_B(timeA) = Second(timeA)
compute_time_B_2(timeA) = Second(Dates.value(timeA))

When using compute_time_B everything is as expected.

julia> A()
A(Second(3600), Second(3600))

But, when using compute_time_B_2 instead, the value is send to compute_time_B_2 before the conversion, so this will yield:

Base.@kwdef struct B
    timeA::Second = Hour(1)
    timeB::Second = compute_time_B_2(timeA)
end 

julia> B()
B(Second(3600), Second(1))

Just something to watch out for when adapting the code.

[milankl]

Okay this is unexpected ☝🏼 But I think this is because the @kwdef macro essentially creates a generator function like so

function B()
    timeA = Hour(1)
    timeB = Second(timeA.value))
    return B(timeA,timeB)
end

[maximilian-gelbrecht]

I really like how all the changes in #418 turned out.

There's one thing we could still adjust to be really consequential and make all user-facing parameters Dates: also make the n_days in run! adhere to Dates format, this would then also change a few things around Clock. In turn this would also allow for simulations that are not full days (not that this is super necessary, but it's a small co-benefit.

What do you think? I could make a PR this, if you want.

[milankl]

I thought about it, but left it for later, because it's much more a breaking change. FYI you can already do n_days=2.5, but I agree that it would be nice to say run!(simulation,for=Day(2)+Hour(12)) but for is obv not allowed. I've been wondering what we could generally use, maybe period, until sounds like you should specify the end date (which I don't think is better) or simply time. I think Oceananigans uses stop_time but time is also treated more generally in there. So maybe period=Day(10) or period=Hour(12) is my preference?

[maximilian-gelbrecht]

I also would say period. I would also still include the n_days with a deprecation warning.

[milankl]

Good call! n_days would give a warning but then period=Day(n_days) which will currently convert to seconds with an info about rounding and conversion (the Dates package throws an error which I find inconvenient). We now simply give back Seconds if you call Day(::AbstractFloat)

[maximilian-gelbrecht]

Closed with #418 #420