-
Notifications
You must be signed in to change notification settings - Fork 191
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
Improves getting time step from Simulation type #649
Conversation
Codecov Report
@@ Coverage Diff @@
## master #649 +/- ##
==========================================
- Coverage 78.03% 78.00% -0.04%
==========================================
Files 118 118
Lines 2363 2364 +1
==========================================
Hits 1844 1844
- Misses 519 520 +1 Continue to review full report at Codecov.
|
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.
Looks good, though there should be a docstring if this is user-facing.
Another possibility is to use Val
to overload getproperty
as we do for boundary condition directions:
I changed the title --- julia has types, but does not have classes. |
There are instances in the code where |
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.
Yeah this is nice but ideally users should be able to just do simulation.Δt
and get the current time step. Unfortunately get_Δt
seems necessary and feels like a Java style getter-setter.
We do need in a few places:
https://github.com/climate-machine/Oceananigans.jl/blob/2ae1cbb86f484420dd2233745a5928a00bf6f77d/src/Simulations.jl#L156
https://github.com/climate-machine/Oceananigans.jl/blob/2ae1cbb86f484420dd2233745a5928a00bf6f77d/src/Simulations.jl#L164
I think we should merge this PR but maybe figure out if there's an easier way to interact with simulations. Printing progress messages seems particularly cumbersome as you end up having to dive into multiple layers of types to get what you want (as shown by the example in #648).
Fixes #648