-
Notifications
You must be signed in to change notification settings - Fork 5
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
run slabplanet on gpu #529
Conversation
|
3a8caa4
to
6cf7daf
Compare
43882fa
to
21ce3e8
Compare
T_sfc_0 = FT(271.0) | ||
if land_temperature_anomaly == "zonally_asymmetric" | ||
Y.bucket.T = map(coords.subsurface) do coord | ||
radlat = coord.lat / FT(180) * pi | ||
ΔT = FT(0) | ||
|
||
anom_ampl = FT(0)# this is zero, no anomaly | ||
lat_0 = FT(60) / FT(180) * pi | ||
lon_0 = FT(-90) / FT(180) * pi | ||
radlon = coord.long / FT(180) * pi | ||
stdev = FT(5) / FT(180) * pi | ||
ΔT = anom_ampl * exp(-((radlat - lat_0)^2 / 2stdev^2 + (radlon - lon_0)^2 / 2stdev^2)) | ||
elseif land_temperature_anomaly == "aquaplanet" | ||
T_sfc_0 + ΔT | ||
end | ||
elseif land_temperature_anomaly == "aquaplanet" | ||
Y.bucket.T = map(coords.subsurface) do coord | ||
ΔT = FT(29) * exp(-coord.lat^2 / (2 * 26^2)) | ||
elseif land_temperature_anomaly == "amip" | ||
T_sfc_0 + ΔT | ||
end | ||
elseif land_temperature_anomaly == "amip" | ||
Y.bucket.T = map(coords.subsurface) do coord | ||
ΔT = FT(40 * cos(radlat)^4) | ||
else | ||
T_sfc_0 + ΔT | ||
end | ||
else | ||
Y.bucket.T = map(coords.subsurface) do coord | ||
ΔT = FT(0) | ||
T_sfc_0 + ΔT | ||
end | ||
T_sfc_0 + ΔT |
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 section could be improved:
- There's lots of hardcoded values (and there's no explanation where they come from
- You are doing dispatch over strings that identify a small set with finite number of possible options, with a fallback. There are two problems with this:
- The fallback might lead to bugs that are very hard to find. For example,
land_temperature_anomaly
were "zonaly_asymmetric" (ie, there's a typo), the code will follow theelse
branch and do something completely different compared to the expected behavior. - Even without the fallback, the pattern is not very robust/scalable, and doesn't provide filesafes. The standard way to do this in other languages is to use
[Enum](https://docs.julialang.org/en/v1/base/base/#Base.Enums.@enum)
s where you define the set of allowed options. I don't think this is done as much in Julia, where we prefer dispatching over types. The advantage of usingEnum
s is that they scale better with more options, do not rely on strings. They also provide a clear place where all the options are collected.
- The fallback might lead to bugs that are very hard to find. For example,
- There's a lot of redundancy that can be removed. Instead of putting the
map
in each branch, you could do something like this:
aquaplanet_T(coord) = XXX
amip_T(coord) = YYY
[...]
T_functions = Dict("aquaplanet" => aquaplanet_T, "amip" => ampi_T, ....)
haskey(T_functions, land_temperature_anomaly) || error("T function not supported")
T_func = T_functions[land_temperature_anomaly]
Y.bucket.T .= T_func.(coords.subsurface)
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.
@LenkaNovak I'm not sure where the hardcoded values come from, do you have any input?
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.
It comes from various experiments we did to match observed or ClimaAtmos prescribed values as initial conditions. zonally_asymmetric
is not being exercised anymore, so feel free to remove it. aquaplanet
follows ClimaAtmos PrognosticSurface initial condition (@szy21 may know which paper these values come from, or if it's based on climatology) and amip
vaguely follows the observed temperature and is what was found most stable (we could replace this with a file read as well, but not as part of this PR).
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.
Zonal symmetric aquaplanet SST is from the moist held-suarez paper eq. 6: https://gmd.copernicus.org/articles/9/1263/2016/gmd-9-1263-2016.pdf
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.
We should document this in Atmos as well. :)
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 added some comments explaining the aquaplanet and AMIP cases, and removed the zonally asymmetric case. Let me know if the comments aren't sufficient
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.
After addressing Gabriele's comments, I only have a few minor ones, thank you, @juliasloan25. I'm also noting a small difference in the AMIP paperplots from the last merge. It doesn't look wrong, it's just slightly different. Would we not expect no behavioral change?
Do we want to open a new issue on adding the albedo read from file to the GPU AMIP runs, once that's sorted in ClimaLSM?
d613b0e
to
ccb772e
Compare
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.
LGTM, @juliasloan25. Following @Sbozzolo 's comments, I only have minor comments. We should also open an issue on reducing the allocations with temporaty fields, if we all agree this would help. Thank you!
T_sfc_0 = FT(271.0) | ||
if land_temperature_anomaly == "zonally_asymmetric" | ||
Y.bucket.T = map(coords.subsurface) do coord | ||
radlat = coord.lat / FT(180) * pi | ||
ΔT = FT(0) | ||
|
||
anom_ampl = FT(0)# this is zero, no anomaly | ||
lat_0 = FT(60) / FT(180) * pi | ||
lon_0 = FT(-90) / FT(180) * pi | ||
radlon = coord.long / FT(180) * pi | ||
stdev = FT(5) / FT(180) * pi | ||
ΔT = anom_ampl * exp(-((radlat - lat_0)^2 / 2stdev^2 + (radlon - lon_0)^2 / 2stdev^2)) | ||
elseif land_temperature_anomaly == "aquaplanet" | ||
T_sfc_0 + ΔT | ||
end | ||
elseif land_temperature_anomaly == "aquaplanet" | ||
Y.bucket.T = map(coords.subsurface) do coord | ||
ΔT = FT(29) * exp(-coord.lat^2 / (2 * 26^2)) | ||
elseif land_temperature_anomaly == "amip" | ||
T_sfc_0 + ΔT | ||
end | ||
elseif land_temperature_anomaly == "amip" | ||
Y.bucket.T = map(coords.subsurface) do coord | ||
ΔT = FT(40 * cos(radlat)^4) | ||
else | ||
T_sfc_0 + ΔT | ||
end | ||
else | ||
Y.bucket.T = map(coords.subsurface) do coord | ||
ΔT = FT(0) | ||
T_sfc_0 + ΔT | ||
end | ||
T_sfc_0 + ΔT |
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.
We should document this in Atmos as well. :)
FT = eltype(area_fraction) | ||
|
||
# atmospheric surface density | ||
Interfacer.update_field!(sim, Val(:air_density), csf.ρ_sfc) | ||
|
||
# turbulent fluxes | ||
mask = Regridder.binary_mask.(area_fraction) |
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 probably allocates as it is called every dt. I wonder if we should have a field in cache which holds temporary calculations on the boundary space, like Atmos does. @Sbozzolo , what do you think? There will be many instances of this though, so I'd suggest leaving this for separate PR.
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, good point (for a future PR)!
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 opened an issue here #584. I can change this back to calling binary_mask
inside the update_field!
calls. I thought this way was more readable, but I agree we should avoid extra allocations.
Just opened an issue for it #583 |
6d31acc
to
e77a41d
Compare
Purpose
go through
coupler_driver_modular.jl
, update functions that won't run on GPU so that the slabplanet setup will run on GPU.Status
This currently runs on
clima
, but not oncentral
due to a too-large parameter getting passed to the gpuSee CliMA/ClimaCore.jl#1597
closes #530
To-do
Content
mpi_init
unique_nodes
errorFT
as a argument (try to get from e.g. space instead)pi
is of typeIrrational(:pi)
, which isn't isbits (should be wrapped in FT)binary_mask
is notisbits
threshold = eps(FT)
or0
, can replace withclamp
, but need other solution for other thresholdsanomaly = false
alwaysSpecific todos (later work - documented in SDI)
FT
passed as argumentpi
not wrapped in FTNotes
BulkAlbedoFunction
, which is GPU-compatible