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

Allow for DateTime clocks #632

Merged
merged 10 commits into from
Mar 2, 2020
Merged

Allow for DateTime clocks #632

merged 10 commits into from
Mar 2, 2020

Conversation

ali-ramadhan
Copy link
Member

Should be useful for LESbrary simulations whose states might actually have an associated date and time from the real world.

But I'm not sure if we should merge this PR as DateTime from Base.Dates only has millisecond precision so you could get small errors in actual model.clock.time after many iterations (LESbrary simulations will probably run for millions of iterations).

Two small packages that might help us here
https://github.com/JeffreySarnoff/TimesDates.jl
https://github.com/FugroRoames/Chrono.jl

@glwagner
Copy link
Member

I agree that something like this would be pretty convenient.

@codecov
Copy link

codecov bot commented Feb 29, 2020

Codecov Report

Merging #632 into master will increase coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #632      +/-   ##
==========================================
+ Coverage   78.03%   78.16%   +0.12%     
==========================================
  Files         118      118              
  Lines        2363     2395      +32     
==========================================
+ Hits         1844     1872      +28     
- Misses        519      523       +4     
Impacted Files Coverage Δ
src/Simulations.jl 93.10% <0.00%> (-2.73%) ⬇️
src/OutputWriters/checkpointer.jl 92.00% <0.00%> (+0.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ae1cbb...396d43d. Read the comment docs.

@ali-ramadhan
Copy link
Member Author

So I modified this PR to support more general AbstractTime clocks. So now clocks work if you pass in a DateTime from Base.Dates (with millisecond precision) but also if you pass in a TimeDate from TimesDates.jl (with nanosecond precision) as they're both subtypes of Dates.AbstractTime.

Oceananigans does not need to depend on TimesDates.jl but I added it as a test dependency so we can make sure TimeDate always works as I think it will be useful for LESbrary simulations.

clock.iteration += 1
return nothing
end

Copy link
Member

Choose a reason for hiding this comment

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

could also call this “tick!” But looks good

Copy link
Member Author

Choose a reason for hiding this comment

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

Love it!

@ali-ramadhan ali-ramadhan merged commit d8cf85b into master Mar 2, 2020
@ali-ramadhan ali-ramadhan deleted the ar/datetime-clocks branch March 2, 2020 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants