-
Notifications
You must be signed in to change notification settings - Fork 23
Reduce use of ᶜspecific #3857
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
base: main
Are you sure you want to change the base?
Reduce use of ᶜspecific #3857
Conversation
cf2a81a
to
8edcb24
Compare
8edcb24
to
14900d3
Compare
19c86c9
to
4c89018
Compare
4c89018
to
b761ab8
Compare
20bc506
to
dd0fce0
Compare
1704408
to
ceed2c9
Compare
@tapios, I tried lumping everything into the one function, but I'm getting an error: ERROR: LoadError: The function body AST defined by this @generated function is not pure. This likely means it contains a closure, a comprehension or a generator.
| Stacktrace:
| [1] macro expansion
| @ /central/scratch/esm/slurm-buildkite/climaatmos-ci/24693/climaatmos-ci/src/prognostic_equations/hyperdiffusion.jl:245 [inlined]
| Is it alright if I break this back down into smaller function calls (which worked) and only do the generated function where it's needed? Or maybe @dennisYatunin has a better idea? |
I'd like to understand what fails here. The problem is in hyperdiffusion, l. 245. Nothing here needs to change if what we use is the old I'm fine with breaking this down into smaller functions, but I'm afraid the problem is something else. @dennisYatunin can weigh in. |
@@ -168,6 +168,33 @@ omitted from the result. | |||
return :(NamedTuple{$specific_gs_names}(($(specific_gs_values...),))) | |||
end | |||
|
|||
""" |
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 need to remove the duplicative specific_gs
above. It should be the same thing as all_specific_gs
.
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 recommend we leave the duplicate definition until we are able to fully remove ᶜspecific
from the cache, at which point we can remove specific_gs
trivially (as nothing will depend on it). If I were to try to remove it right now, it would take me some time to figure out how to make all_specific_gs
be able to return a field (all_specific_gs
returns a NamedTuple
of Broadcasted
objects) like specific_gs
does.
Isn't the right thing to do here is using the same logic of looping over scalar names that you used several times in #3858? |
|
As I mentioned, you'd want to use a (corrected form) of |
It's not clear to me how to use for χ_name in propertynames(ᶜ∇²specific_tracers)
@. ᶜ∇²specific_tracers.:($$χ_name) = wdivₕ(gradₕ(ᶜspecific.:($$χ_name)))
end ? |
No description provided.