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

Improve Eady figure #145

Merged
merged 14 commits into from
Sep 24, 2023
Merged

Improve Eady figure #145

merged 14 commits into from
Sep 24, 2023

Conversation

jagoosw
Copy link
Collaborator

@jagoosw jagoosw commented Sep 18, 2023

This PR addresses #140 and increases the model resolution.

Closes #140

paper/paper.md Outdated
@@ -72,7 +72,7 @@ An example of a problem involving small-scale flow features is showcased in \aut
![In this simulation of baroclinic instability in the Eady problem, a background buoyancy gradient and corresponding thermal wind generates a sub-mesoscale eddy, roughly following the setup of @taylor:2016.
To this physical setup, we added a medium complexity (9 tracers) biogeochemical model, some components of which are shown above.
On top of this, we added particles modelling the growth of sugar kelp, which are free-floating and advected by the flow, and carbon dioxide exchange from the air.
Thanks to Julia's speed and efficiency the above model (1 km × 1 km × 100 m with 64 × 64 × 16 grid points) took about 30 minutes of computing time to simulate 10 days of evolution on an Nvidia P100 GPU.
Thanks to Julia's speed and efficiency the above model (1 km × 1 km × 100 m with 512 × 512 × 64 grid points) took about X minutes of computing time to simulate 10 days of evolution on an Nvidia A100 GPU.
Panel (a) shows the domain with the colour representing the concentration of various biogeochemical tracer fields: inorganic carbon, organic carbon (dissolved and particulate), phytoplankton, and nutrients.
The increase in organic carbon concentration in the centre of the eddy can be seen, as well as carbon being subducted (most visible in the xz face in the organic carbon).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@johnryantaylor I will update this caption when I have made the new figure, but do you want to add a sentence about the physics visible at the high resolution?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point - I suggested a sentence above

paper/paper.md Outdated Show resolved Hide resolved

u, v, w = model.velocities

# Periodically save the velocities and vorticity to a file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Periodically save the velocities and vorticity to a file.
# Periodically save the velocities to a file.


n = 32

x = arch_array(arch, [repeat([-200.0, -100.0, 0.0, 100.0, 200.0], 1, 5)...] .+ Lx / 2)
Copy link
Collaborator

@navidcy navidcy Sep 19, 2023

Choose a reason for hiding this comment

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

Suggested change
x = arch_array(arch, [repeat([-200.0, -100.0, 0.0, 100.0, 200.0], 1, 5)...] .+ Lx / 2)
x = arch_array(arch, [repeat([-200.0, -100.0, 0.0, 100.0, 200.0], 1, 5)...] .+ Lx / 2)

Also, where does this 5 come from? Seems hardcoded. Should we define a variable so that it's clearer how one can change it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll update this since there was an issue here too


particles = SLatissima(; architecture = arch,
x, y, z,
A = arch_array(arch, 5.0 .* ones(n)), N = arch_array(arch, 0.01.* ones(n)), C = arch_array(arch, 0.18.* ones(n)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
A = arch_array(arch, 5.0 .* ones(n)), N = arch_array(arch, 0.01.* ones(n)), C = arch_array(arch, 0.18.* ones(n)),
A = arch_array(arch, 5.0 .* ones(n)), N = arch_array(arch, 0.01 .* ones(n)), C = arch_array(arch, 0.18 .* ones(n)),

Comment on lines 126 to 128
schedule = TimeInterval(6hours),
filename = "eady_particles.jld2",
overwrite_existing = true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
schedule = TimeInterval(6hours),
filename = "eady_particles.jld2",
overwrite_existing = true)
schedule = TimeInterval(6hours),
filename = "eady_particles.jld2",
overwrite_existing = true)

particles = SLatissima(; architecture = arch,
x, y, z,
A = arch_array(arch, 5.0 .* ones(n)), N = arch_array(arch, 0.01.* ones(n)), C = arch_array(arch, 0.18.* ones(n)),
latitude = 57.5,
Copy link
Collaborator

Choose a reason for hiding this comment

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

where does this latitude value comes from? is this the value that corresponds to the f value we use above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was arbitrary for fast kelp growth, but I've changed now to correspond to the coriolis force.

paper/paper.md Outdated
@@ -72,7 +72,7 @@ An example of a problem involving small-scale flow features is showcased in \aut
![In this simulation of baroclinic instability in the Eady problem, a background buoyancy gradient and corresponding thermal wind generates a sub-mesoscale eddy, roughly following the setup of @taylor:2016.
To this physical setup, we added a medium complexity (9 tracers) biogeochemical model, some components of which are shown above.
On top of this, we added particles modelling the growth of sugar kelp, which are free-floating and advected by the flow, and carbon dioxide exchange from the air.
Thanks to Julia's speed and efficiency the above model (1 km × 1 km × 100 m with 64 × 64 × 16 grid points) took about 30 minutes of computing time to simulate 10 days of evolution on an Nvidia P100 GPU.
Thanks to Julia's speed and efficiency the above model (1 km × 1 km × 100 m with 512 × 512 × 64 grid points) took about X minutes of computing time to simulate 10 days of evolution on an Nvidia A100 GPU.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"X minutes" needs to be replaced with the number

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah not quite finished running!

Copy link
Collaborator

@navidcy navidcy left a comment

Choose a reason for hiding this comment

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

some remarks

@jagoosw jagoosw marked this pull request as ready for review September 24, 2023 13:22
@jagoosw
Copy link
Collaborator Author

jagoosw commented Sep 24, 2023

Do we think the eady plot is now sufficiently clear and large enough text?

@jagoosw
Copy link
Collaborator Author

jagoosw commented Sep 24, 2023

It doesn't have the colorbars or kelp line plot but I don't think they add much to the demonstration value of the plot.

@jagoosw
Copy link
Collaborator Author

jagoosw commented Sep 24, 2023

I think I would like to add to one of the sentences about performance an acknowledgement of and reference for KA/JuliaGPU

@jagoosw
Copy link
Collaborator Author

jagoosw commented Sep 24, 2023

I've also just noticed that the reference for Oceananigans isn't at the first instance it was mentioned, should we change?

@navidcy
Copy link
Collaborator

navidcy commented Sep 24, 2023

Do we think the eady plot is now sufficiently clear and large enough text?

Fontsizes for axis labels don't seem to have increased. Have they?
Also lack of colorbars makes it less clear for me.
Another suggestion is to alter a bit the colorrange for the left-most part of the figure so that the y-z structure shows up better?

Comment on lines +207 to +209
sediment.denitrification_params.E * log(reactivity) ^ 2 +
sediment.denitrification_params.F * log(O₂) * log(reactivity)) / (Cᵐⁱⁿ * day)
=#
Copy link
Collaborator

Choose a reason for hiding this comment

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

shall we delete these commented code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess so, might be better todo in #138 since I've made a lot of changes to the sediment there

@jagoosw
Copy link
Collaborator Author

jagoosw commented Sep 24, 2023

Fontsizes for axis labels don't seem to have increased. Have they? Also lack of colorbars makes it less clear for me. Another suggestion is to alter a bit the colorrange for the left-most part of the figure so that the y-z structure shows up better?

I think the axis labels appear the same size as other figures now? I think the main complaint was about the ticks.

Is this better with color bars:
eady

I've adjusted the colorrange so it clips on the visible faces (rather than the whole volumes min/max), is that okay?

@navidcy
Copy link
Collaborator

navidcy commented Sep 24, 2023

Fontsizes for axis labels don't seem to have increased. Have they? Also lack of colorbars makes it less clear for me. Another suggestion is to alter a bit the colorrange for the left-most part of the figure so that the y-z structure shows up better?

I think the axis labels appear the same size as other figures now? I think the main complaint was about the ticks.

Oh I see! Okie then!

Is this better with color bars: eady

Yes! Much better!

I've adjusted the colorrange so it clips on the visible faces (rather than the whole volumes min/max), is that okay?

Try 0.8*min / max ? Ideally I'd like a bit more contrast for the left-part of the figure. But it's also fine as is!

@jagoosw
Copy link
Collaborator Author

jagoosw commented Sep 24, 2023

I think this is a bit better:
eady_example
Its kind of hard to make some of these colormaps give more contrast because they're meant to be perceptually uniform

@jagoosw jagoosw merged commit 591d3d2 into joss-paper Sep 24, 2023
1 check passed
@jagoosw jagoosw deleted the jsw/joss-eady-fig branch September 24, 2023 18:04
@navidcy navidcy mentioned this pull request Sep 26, 2023
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.

3 participants