-
Notifications
You must be signed in to change notification settings - Fork 17
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
A GPU compatible version for non-orographic gravity wave parameterization #3267
Conversation
This code is really complex, has little to no comments that explain what is happening, uses advanced features and introduces a dependency on UnrolledUtilities, a package that is not documented in such a way that people can understand what it does and when/how to use it without in-person guidance. I am worried that almost no one will be able to extend and maintain this piece of code. @dennisYatunin can you help reducing complexity and improving clarity? If you really want to use |
34e19c5
to
3e39e7a
Compare
0b37734
to
4b5522a
Compare
15a7648
to
92c4fbc
Compare
dt_save_state_to_disk: "400secs" | ||
initial_condition: "IsothermalProfile" | ||
t_end: "1500secs" | ||
hyperdiff: false |
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.
dt_save_state_to_disk: "400secs" | |
initial_condition: "IsothermalProfile" | |
t_end: "1500secs" | |
hyperdiff: false | |
t_end: "1500secs" |
6987d2a
to
8f9fd3c
Compare
Operators.placeholder_space(current_space, parent_space) = | ||
current_space isa Spaces.AbstractSpace && | ||
parent_space isa Spaces.AbstractSpace && | ||
Spaces.issubspace(current_space, parent_space) ? | ||
Operators.PlaceholderSpace() : current_space |
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.
This overrides a method in ClimaCore. If this is needed, it needs to be put into ClimaCore.
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.
Yes, it is needed. Otherwise the code could not run on GPU. Dennis tells me he will open a pull request in ClimaCore soon.
function calc_intermitency(ρ_source_level, source_ampl, nk, Bsum) | ||
return (source_ampl / ρ_source_level / nk) / Bsum | ||
end | ||
|
||
function gw_average!(wave_forcing, wave_forcing_m1) | ||
L1 = Operators.LeftBiasedC2F(; bottom = Operators.SetValue(0.0)) | ||
L1 = Operators.LeftBiasedC2F(; bottom = Operators.SetValue(0.0f0)) |
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.
Can we not hard-code Float32 here? (and below)
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.
Sure! I will change it.
@. uforcing = uforcing + u_waveforcing | ||
@. vforcing = vforcing + v_waveforcing | ||
|
||
end | ||
return nothing | ||
end | ||
|
||
# calculate the intermittency factor eps -> assuming constant Δc. | ||
# Using column_accumulate function, calculate the gravity wave forcing at each point. | ||
function waveforcing_column_accumulate!( |
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 not keen on nested anonymous functions like this. Can we somehow break the contents of this giant function into smaller function calls?
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've refactored part of the wave source calculation into smaller function calls. Does this approach work for you? Breaking down the unrolled_reduce function, which is a big part of the whole function, isn’t easy because it directly uses many externally passed parameters. If we avoid using anonymous functions, we’ll have to keep passing those parameters in the reduce operation, which might make the function less elegant.
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.
wave_source
looks great, thanks for updating that. It would be nice if it's possible to do something similar with the block inside the unrolled_reduce(Val(nc), (mask, FT1(0.0))) do (mask, fm), (n)
block, but I understand if it's not possible.
1553bfb
to
f977bfc
Compare
f977bfc
to
68c6b32
Compare
68c6b32
to
47e0e20
Compare
Purpose
Let Nogw parameterization work on GPU
To-do
Speed it up (Now this version now runs 50% slower than the previous version of the code.)
Content
Replace the for-end loop structure with unrolled functions