Includes simulation tips section in the docs #1543

tomchor · 2021-04-04T16:26:14Z

As mentioned in #1542 I couldn't build the docs locally so I probably made some markdown mistakes along the way, which is why this is a draft pull request. This first draft is also somewhat incomplete.

I figured I'd create the PR early to get some feedback along the way though. I'm especially unfamiliar with the part about converting arrays to CUDA etc., so some help/collaboration there is much appreciated.

CC @ali-ramadhan @glwagner @navidcy @francispoulin

francispoulin · 2021-04-05T12:48:25Z

docs/src/simulation_tips.md

+using Oceananigans.Grids: Center, Face
+using Oceananigans.Fields: KernelComputedField
+
+@inline ψ²(i, j, k, grid, ψ) = @inbounds ψ[i, j, k]^2


You did a great job in explaining @inline above. Is it worth having a sentence explaining @inbounds as well?

I don't know much about @inbounds in Julia, but from what I can tell the user doesn't need to use it that much (except when creating KernelComputedFields, which has many other difficulties). So I'd say we don't need to touch on that here (maybe in docs for KernelComputedField). But I might be missing something.

Sadly, I don't remember when to use @inbuonds and when not to use it. @ali-ramadhan explained it to me once. Maybe he could help in adding a sentence? I would certainly use this when writing code as at the moment it seems mysterious to me when you use it and when you don't.

It just tell julia to not bother checking whether the index is beyond the array’s limit. At least that’s what I think it does...

https://docs.julialang.org/en/v1/devdocs/boundscheck/

docs/src/simulation_tips.md

francispoulin

I think this is a reallly nice contribution and I learned some things along the way so thanks.

I have still a lot to learn about efficiency so will let someone else who is more knowledgeable on the topic approve and add more feedback

glwagner · 2021-04-05T13:27:21Z

docs/src/simulation_tips.md

+In practice it's hard to say whether inlining a function will bring runtime benefits _with
+certainty_, since Julia already inlines some small functions automatically. However, it is generally
+a good idea to at least investigate this aspect in your code as the benefits can potentially be
+significant.


It may be worth mentioning that KernelAbstractions "force inlines" all GPU code. I can't remember if CPU code is also force inlined. @vchuravy and @jakebolewski have give advice regarding the use of @inline in the past. Force-inlining suggests that we don't always need the annotation @inline but for some reason it still pervades all our code... ?

I added a suggestion to mention KernelAbstractions.jl but it might not be useful to discuss inlining behavior to new users (many of whom are completely new to Julia).

My (probably unpopular) opinion is that I like to explicitly @inline functions to tell people reading my code that this is a performance-critical function I intend to have inlined whenever possible (even if it'll be force-inlined by the compiler). But this is probably a debate for another place.

I think the docs on inlining in Julia are a bit scarce. There's a lot of spread out in discourse comments. For the time being I think this is the limit of my knowledge. But feel free to adapt the text if you think it needs more details.

I think its fine to recommend @inline for style / clarity. But we shouldn't mislead people in the docs by saying that @inline is necessary for performance if it has no effect.

@glwagner are you suggesting that @inline doesn't impact performance?

I don't think it does for GPU, but it might for CPU!

Could be fun to illustrate these points with code snippets that do some benchmarking that users can use for themselves (some day).

docs/src/simulation_tips.md

glwagner · 2021-04-05T13:29:45Z

Thanks for starting this @tomchor .

ali-ramadhan · 2021-04-05T16:41:25Z

Docs should show up at https://clima.github.io/OceananigansDocumentation/previews/PR1543/ once a commit has been made after the PR has been opened.

docs/src/simulation_tips.md

ali-ramadhan · 2021-04-05T17:00:27Z

docs/src/simulation_tips.md

+In practice it's hard to say whether inlining a function will bring runtime benefits _with
+certainty_, since Julia already inlines some small functions automatically. However, it is generally
+a good idea to at least investigate this aspect in your code as the benefits can potentially be
+significant.


I added a suggestion to mention KernelAbstractions.jl but it might not be useful to discuss inlining behavior to new users (many of whom are completely new to Julia).

My (probably unpopular) opinion is that I like to explicitly @inline functions to tell people reading my code that this is a performance-critical function I intend to have inlined whenever possible (even if it'll be force-inlined by the compiler). But this is probably a debate for another place.

docs/src/simulation_tips.md

ali-ramadhan · 2021-04-05T17:04:46Z

docs/src/simulation_tips.md

+
+### Arrays in GPUs are usually different from arrays in CPUs
+
+Talk about converting to CuArrays and viewing CuArrays as well!


Should we also talk about CUDA scalar operations (https://juliagpu.github.io/CUDA.jl/dev/usage/workflow/#UsageWorkflowScalar) and how/when to use CUDA.allowscalar and CUDA.@allowscalar?

My guess is yes, but I know nothing about this topic, so some collaboration would be very much appreciated :)

@ali-ramadhan

@ali-ramadhan's suggestion Co-authored-by: Ali Ramadhan <[email protected]>

Co-authored-by: Gregory L. Wagner <[email protected]>

Co-authored-by: Ali Ramadhan <[email protected]>

tomchor · 2021-04-05T17:39:00Z

Docs should show up at https://clima.github.io/OceananigansDocumentation/previews/PR1543/ once a commit has been made after the PR has been opened.

This gives me a 404 error!

navidcy · 2021-04-05T23:49:41Z

Docs should show up at https://clima.github.io/OceananigansDocumentation/previews/PR1543/ once a commit has been made after the PR has been opened.

This gives me a 404 error!

I believe you have to open the PR (not have it as "Draft") and then it'll push the preview....

tomchor · 2021-04-07T01:19:35Z

I finished the first draft (of the topics I know about at least). I haven't written the very last subsection though, which is about viewing/using arrays in GPU runs because honestly I don't know enough to write about it. I know there's a function called view() that helps with slicing, and you can also use adapt to view CUDA Arrays in REPL for investigating, but I feel like I don't know enough of this aspect of the code to write this. There are two options here:

Someone helps me out and writes that last section
Or I can merge the PR without it for now (there are many other important things there for the user) and we worry about that last part in a future PR

Thoughts?

glwagner · 2021-04-12T19:31:54Z

I'm wondering if we should provide a separate page on "Using GPUs"? While the simulation tips for CPUs are really performance optimizations that are optional, the GPU simulation tips are mostly required to run without errors. There's a few other things that are required to get things working on GPUs --- for example, Oceananigans must be built (not just run) with a GPU / CUDA installation available; this is a common pitfall on clusters.

tomchor · 2021-04-13T01:28:56Z

I'm wondering if we should provide a separate page on "Using GPUs"? While the simulation tips for CPUs are really performance optimizations that are optional, the GPU simulation tips are mostly required to run without errors.

That's a good point. Although I think we could avoid creating another page and put that information in the "Using GPUs" page, so that things are more condensed.

glwagner · 2021-04-13T01:39:59Z

I'm wondering if we should provide a separate page on "Using GPUs"? While the simulation tips for CPUs are really performance optimizations that are optional, the GPU simulation tips are mostly required to run without errors.

That's a good point. Although I think we could avoid creating another page and put that information in the "Using GPUs" page, so that things are more condensed.

🤦 there's already Using GPUs of course, silly me...

navidcy · 2021-04-14T03:56:00Z

We have a preview!
https://clima.github.io/OceananigansDocumentation/previews/PR1543/simulation_tips/
Seems like the section needs to be finished (see, e.g., last sentence...). @tomchor?

docs/src/simulation_tips.md

navidcy · 2021-04-14T04:01:56Z

docs/src/simulation_tips.md

+
+It may be useful to know that there are some kernels already defined for commonly-used diagnostics
+in packages that are companions to Oceananigans. For example
+[Oceanostics.jl](https://github.com/tomchor/Oceanostics.jl/blob/13d2ba5c48d349c5fce292b86785ce600cc19a88/src/TurbulentKineticEnergyTerms.jl#L23-L30)


I'm a bit hesitant to reference the users to a package that is neither registered nor tested. (mostly the latter tbh)
What do others think?

Oceanostics.jl is registered. It could do with some actual tests but plenty of Oceananigans.jl functionality is not tested.

I think it's good to link to packages outside of Oceananigans.jl to give readers the idea that creating their own packages for extra functionality is okay (and actually encouraged!).

I would even advocate for creating an extra page in the docs to link to other packages related to Oceananigans.jl and being liberal with the packages we include.

Co-authored-by: Navid C. Constantinou <[email protected]>

tomchor · 2021-04-14T13:30:13Z

Thanks for the help with this PR, @navidcy! Much appreciated

Seems like the section needs to be finished (see, e.g., last sentence...). @tomchor?

Ah, that's the limit of my knowledge on GPUs. We could either drop that session for now or someone else could write it. I really don't think I know enough to write that section.

ali-ramadhan

I added some quick text under the last section. Should be better than nothing.

Happy to merge this as-is now since it's already a great addition to the docs that we can continue expanding in the future as common GPU issues pop up.

docs/src/simulation_tips.md

ali-ramadhan · 2021-04-14T13:54:22Z

I think GPU tests randomly crapped out (not important for this PR) but docs are still building so as long as the PR docs preview looks good, then this should be good to merge!

tomchor · 2021-04-14T14:16:26Z

@ali-ramadhan currently waiting for the preview to load so that I can merge, but I don't think it'll do that with the GPU test failing. How do we fix that?

ali-ramadhan · 2021-04-14T14:26:22Z

@tomchor Ah are you okay with committing my suggestion before merging?

I think docs did get built and deployed:
https://buildkite.com/clima/oceananigans/builds/2048#0d49a5b8-1dec-4f7b-b82a-cf6f8809e234/40-574
https://clima.github.io/OceananigansDocumentation/previews/PR1543/simulation_tips/

Working on getting you guys access to Buildkite so you can control it as well. I'm not a Buildkite admin so I have to ask one to invite other people...

Co-authored-by: Ali Ramadhan <[email protected]>

tomchor · 2021-04-14T14:29:23Z

@tomchor Ah are you okay with committing my suggestion before merging?

Sorry, I forgot to do that!

ali-ramadhan · 2021-04-14T15:14:53Z

Everything passed 🎉 Thanks so much for adding this great information to the docs @tomchor!

tomchor · 2021-04-14T15:25:23Z

Thanks, everyone. I'll merge this for now and we can improve on it later based on feedback from users.

@glwagner I'm thinking of opening another PR soon to address your comment about using GPUs. Like I mentioned, some of this info is already available in the "Using GPUs" page, but maybe it's useful to expand a bit on it and link this newly created page there.

started first draft of simulation tips sectino in the docs

d43d65c

francispoulin reviewed Apr 5, 2021

View reviewed changes

docs/src/simulation_tips.md Outdated Show resolved Hide resolved

francispoulin reviewed Apr 5, 2021

View reviewed changes

docs/src/simulation_tips.md Show resolved Hide resolved

francispoulin reviewed Apr 5, 2021

View reviewed changes

docs/src/simulation_tips.md Outdated Show resolved Hide resolved

francispoulin reviewed Apr 5, 2021

View reviewed changes

glwagner reviewed Apr 5, 2021

View reviewed changes

docs/src/simulation_tips.md Outdated Show resolved Hide resolved

ali-ramadhan reviewed Apr 5, 2021

View reviewed changes

tomchor and others added 4 commits April 5, 2021 10:21

Explaining interpolation a bit better in KernelComputedField example

7807a0b

Update docs/src/simulation_tips.md

a93d27e

@ali-ramadhan's suggestion Co-authored-by: Ali Ramadhan <[email protected]>

Update docs/src/simulation_tips.md

6b0f282

Co-authored-by: Gregory L. Wagner <[email protected]>

Update docs/src/simulation_tips.md

184b5e1

Co-authored-by: Ali Ramadhan <[email protected]>

tomchor added 2 commits April 5, 2021 10:44

Included relevant links

34e73e9

resolution -> accuracy

50a880c

navidcy added the documentation 📜 The sacred scrolls label Apr 5, 2021

finished first draft (with the exception of very last subsection)

7a2c24e

tomchor marked this pull request as ready for review April 7, 2021 01:13

tomchor requested review from ali-ramadhan, glwagner and francispoulin April 7, 2021 01:19

Merge branch 'master' into tc/simulation_tips

04669f6

navidcy self-requested a review April 14, 2021 02:34

navidcy requested changes Apr 14, 2021

View reviewed changes

tomchor and others added 3 commits April 14, 2021 06:22

Update docs/src/simulation_tips.md

f0d7275

Co-authored-by: Navid C. Constantinou <[email protected]>

Update docs/src/simulation_tips.md

a5762d2

Co-authored-by: Navid C. Constantinou <[email protected]>

Update docs/src/simulation_tips.md

2f9544c

Co-authored-by: Navid C. Constantinou <[email protected]>

ali-ramadhan approved these changes Apr 14, 2021

View reviewed changes

docs/src/simulation_tips.md Outdated Show resolved Hide resolved

navidcy self-requested a review April 14, 2021 13:51

navidcy approved these changes Apr 14, 2021

View reviewed changes

ali-ramadhan closed this Apr 14, 2021

ali-ramadhan reopened this Apr 14, 2021

Update docs/src/simulation_tips.md

dedea5a

Co-authored-by: Ali Ramadhan <[email protected]>

tomchor merged commit 28dadbc into master Apr 14, 2021

tomchor deleted the tc/simulation_tips branch April 14, 2021 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Includes simulation tips section in the docs #1543

Includes simulation tips section in the docs #1543

tomchor commented Apr 4, 2021 •

edited

Loading

francispoulin Apr 5, 2021

tomchor Apr 5, 2021 •

edited

Loading

francispoulin Apr 7, 2021

navidcy Apr 7, 2021

francispoulin left a comment

glwagner Apr 5, 2021

ali-ramadhan Apr 5, 2021 •

edited

Loading

tomchor Apr 5, 2021

glwagner Apr 6, 2021

tomchor Apr 6, 2021

glwagner Apr 14, 2021 •

edited

Loading

glwagner Apr 14, 2021 •

edited

Loading

glwagner commented Apr 5, 2021

ali-ramadhan commented Apr 5, 2021

ali-ramadhan Apr 5, 2021 •

edited

Loading

ali-ramadhan Apr 5, 2021

tomchor Apr 5, 2021

tomchor commented Apr 5, 2021

navidcy commented Apr 5, 2021

tomchor commented Apr 7, 2021 •

edited

Loading

glwagner commented Apr 12, 2021

tomchor commented Apr 13, 2021

glwagner commented Apr 13, 2021 •

edited

Loading

navidcy commented Apr 14, 2021

navidcy Apr 14, 2021

ali-ramadhan Apr 14, 2021

tomchor commented Apr 14, 2021

ali-ramadhan left a comment

ali-ramadhan commented Apr 14, 2021 •

edited

Loading

tomchor commented Apr 14, 2021

ali-ramadhan commented Apr 14, 2021

tomchor commented Apr 14, 2021

ali-ramadhan commented Apr 14, 2021

tomchor commented Apr 14, 2021


		### Arrays in GPUs are usually different from arrays in CPUs

		Talk about converting to CuArrays and viewing CuArrays as well!

Includes simulation tips section in the docs #1543

Includes simulation tips section in the docs #1543

Conversation

tomchor commented Apr 4, 2021 • edited Loading

Choose a reason for hiding this comment

tomchor Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francispoulin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ali-ramadhan Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glwagner Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

glwagner Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

glwagner commented Apr 5, 2021

ali-ramadhan commented Apr 5, 2021

ali-ramadhan Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomchor commented Apr 5, 2021

navidcy commented Apr 5, 2021

tomchor commented Apr 7, 2021 • edited Loading

glwagner commented Apr 12, 2021

tomchor commented Apr 13, 2021

glwagner commented Apr 13, 2021 • edited Loading

navidcy commented Apr 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomchor commented Apr 14, 2021

ali-ramadhan left a comment

Choose a reason for hiding this comment

ali-ramadhan commented Apr 14, 2021 • edited Loading

tomchor commented Apr 14, 2021

ali-ramadhan commented Apr 14, 2021

tomchor commented Apr 14, 2021

ali-ramadhan commented Apr 14, 2021

tomchor commented Apr 14, 2021

tomchor commented Apr 4, 2021 •

edited

Loading

tomchor Apr 5, 2021 •

edited

Loading

ali-ramadhan Apr 5, 2021 •

edited

Loading

glwagner Apr 14, 2021 •

edited

Loading

glwagner Apr 14, 2021 •

edited

Loading

ali-ramadhan Apr 5, 2021 •

edited

Loading

tomchor commented Apr 7, 2021 •

edited

Loading

glwagner commented Apr 13, 2021 •

edited

Loading

ali-ramadhan commented Apr 14, 2021 •

edited

Loading