-
Notifications
You must be signed in to change notification settings - Fork 190
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
Hello, ocean example #1181
Hello, ocean example #1181
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1181 +/- ##
==========================================
- Coverage 56.99% 50.03% -6.96%
==========================================
Files 162 161 -1
Lines 3916 3849 -67
==========================================
- Hits 2232 1926 -306
- Misses 1684 1923 +239
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.
I like it.
The way it's written though is not ready to go into the literated examples list. But we should include be literating it (even if we decide that we should not include it in the docs) to make sure it can run as the code keeps changing.
I'm a bit conflicted whether we should keep this as simple as is and include it in the README or whether we should add more explanations and include it in the list of literated docs examples... Probably I'd vote for the former since I don't see anything that this example has to offer in terms of demonstrating Oceananigans.jl functionality.
examples/hello_ocean.jl
Outdated
|
||
using Oceananigans, Oceananigans.Grids, Plots | ||
|
||
grid = RegularCartesianGrid(size = (1, 64, 64), |
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.
why not x
and z
? I think that's more natural
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.
because using y, z generalizes to GPU
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.
and x, z
does not?
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.
no, not if x is bounded
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.
you are saying that
grid = RegularCartesianGrid(size = (1, 64, 64),
x = (-4, 4), y = (0, 1), z = (-4, 4),
topology = (Bounded, Periodic, Bounded))
won't run on GPU?
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 topology = (Bounded, Periodic, Bounded)
won't even run on the CPU right now for the same reason as #1007.
I'm working on supporting all topologies in the ali/pressure-solver-tlc
branch so topology = (Bounded, Periodic, Bounded)
works on the CPU there, but still needs more work and TLC for the GPU to support all topologies.
examples/hello_ocean.jl
Outdated
|
||
@info "Simulating a rising buoyant bubble with" model | ||
|
||
set!(model, b = (x, y, z) -> exp(-y^2 - z^2)) |
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.
Is, perhaps, exp(-(y^2 + z^2))
more soothing for the eye?
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.
that's more characters!
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.
my question was whether it's more soothing for the eye not whether it's easier for the fingers of the person who types it...
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.
characters strain the eye, as well as typing fingers...
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.
ok, fair enough.
But it bothers me that y kisses the minus sign before it while there is some distance between - and z... Seems like y and z are not treated equally..
examples/hello_ocean.jl
Outdated
|
||
b = model.tracers.b | ||
|
||
plt = contourf(ynodes(b), znodes(b), interior(b)[1, :, :]', |
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.
Why not an animation?
Or if we just plot, then I suggest we plot both the initial + final snapshot?
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.
animation is too much for "hello, ocean"... ?
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.
could plot initial, yes
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.
i think plotting the initial conditions would add information for sure. however, it would create 3 additional lines at least
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.
hmm, if we make the plot (but don't save the data), and then just combine the plots after, it is just 2 additional lines. perhaps worth it.
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.
I'm definitely on board for more animation but a nice plot should be enough for the first introductory example (initial conditions would be pretty useful I think).
I think the bigger issue with doing animation is it would complicate the example since it either requires an output writer (probably a good thing to show although all the Literate.jl examples show how to use output writers). Without an output writer, the plotting has to happen in a loop that keeps extending simulation.stop_time
which is not a nice pattern.
The purpose of a hello, ocean is to illustrate syntax in a "minimal" example. It's a teaser, something to draw you in. Eg |
@navidcy what do you think about the thermal bubble as the "canonical minimal example" ? Is there something better, more striking, simpler, more fun? |
julia> minimum(interior(b)[1, :, :])
-0.0009534228444548004 Is this OK? |
oscillatory errors? |
Exactly! I agree! That's why I voted for the former, i.e., leave it as is and include it in the README. |
But can we think of other possibilities? An initial value problem seems good, as well as one where "something" happens. It might be more ocean-y if it was convection or something... hmm. |
https://www.youtube.com/watch?v=95zljDYHSqU&feature=emb_title https://grenouillebouillie.wordpress.com/2010/07/24/hello-world/ "Only 23 lines..." but looks like we are at 35 (and no spinning Earth) |
Probably you are correct... The minimum value reduces by a factor of 4 if I use 128x128 points. So let's leave it as is. |
Ah, let's do that! |
It would be a few more lines, but a forcing that creates an internal wave (eg St. Andrews Cross) might be "more canonical"... ? |
That's another good idea. |
I think a really cool one would be some kind of breaking internal pattern that looks like a breaking wave (our signature emoji) |
I actually think I prefer the bubble... |
Do you have any ideas for producing a nice looking breaking wave pattern? |
Can you elaborate? What isn't ready about it? If there are ways to make it shorter or better, we should definitely do that in this PR rather than later! |
I can elaborate. What I meant is that this example, although it's Literate-ready, it does not provide any explanation to the reader about what's coming. All other examples come with remarks of the sort "we now build the grid"... "Next, let's form a model with two tracers...", "now all what's left to do is to run the simulation"... On the other hand, this example just list down the commands without any explanation. I'm not saying that's bad. It's just different from the rest of the examples. I think it's perfect for the README file but. in my opinion, you don't wanna see this as your first example if you are reading the docs because it doesn't really take you from the hand and guide you to feel welcome in all this new Oceananigans.jl terminology. To conclude: I think we should add it to the Literate examples list, e.g., in examples = [
"hello_ocean.jl",
"one_dimensional_diffusion.jl",
"two_dimensional_turbulence.jl",
"internal_wave.jl",
"convecting_plankton.jl",
"ocean_wind_mixing_and_convection.jl",
"langmuir_turbulence.jl",
"eady_turbulence.jl",
"kelvin_helmholtz_instability.jl"
]
for example in examples
example_filepath = joinpath(EXAMPLES_DIR, example)
Literate.markdown(example_filepath, OUTPUT_DIR, documenter=true)
end This way, docs will fail if the code cannot run the That's all what I meant. My 2 cents have been now deposited in this PR. :) |
But this example is our "hello, ocean", which has a different purpose than the others... ? I suspect that different people may want different types of examples, but I'm not sure. I personally like short, revelatory bits of code, especially if I can read the code (using my knowledge of julia and physics) and infer what's going on. Certainly the detailed explanations are nice too, in a different way. Curious what @ali-ramadhan thinks... |
@ali-ramadhan? :) |
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.
My understanding is that this example would replace the rising thermal bubble example in the README https://github.com/CliMA/Oceananigans.jl#more-interesting-example (and maybe also have a corresponding version in the docs?).
I think 43 lines might be a bit much for the introductory README example but should be easy to trim down to ~25 lines I think.
@@ -0,0 +1,43 @@ | |||
# # Hello, ocean! | |||
# | |||
# > How inappropriate to call this planet Earth, when it is quite clearly _Ocean_. |
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.
🌐
# | ||
# --Arthur C. Clark | ||
|
||
using Oceananigans, Oceananigans.Grids, Plots |
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.
Would be ideal to not have Oceananigans.Grids
in the introductory README example (same with Oceananigans.Advection.WENO5
) but we already agreed to fix this in the future so it's good for now.
Σ(ξ) = (1 - tanh(ξ)) / 2 | ||
Π(ξ, δ=1) = (Σ(ξ - δ) - Σ(ξ + δ)) / 2 | ||
|
||
set!(model, | ||
u = (x, y, z) -> Σ(-z), | ||
b = (x, y, z) -> - Π(4x, 1) * Σ(32z)) |
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.
I might give these more self-explanatory names as I imagine they look quite cryptic, especially to new users.
The shorter, the better for hello ocean! This example does a bit more because it adds a plot and uses different numerics (could avoid the need to specify good numerical manually by changing defaults?) I think hello ocean should plot (otherwise there's no "hello") but we could combine some lines for sure. Still not sure the best problem. A rising hot patch produces decent viz but not particularly oceanic. |
Closing as this is stale, but maybe we'll revise this idea someday. |
This PR adds a new minimal example that simulates and plots a rising buoyant bubble:
If we do think an example like this would be nice to have, it might make sense to replace the README example with this one (and perhaps otherwise improve the README).
I didn't add it to the list of examples in the docs, but it is set up (in it's minimal way) for Literate-ing. Or we can un-literate it.
Suggest away and I shall commit!
The script produces
Could also add a
pkg"add Oceananigans, Plots"
at the top per the discussion on #1149, but it might better to unify all the examples at once in a dedicated PR.