Use LazyBroadcast in set_velocity_quantities #3608
Conversation
There was a problem hiding this comment.
It looks mostly correct (although there may be a bug somewhere as some tests failed), but I find it a bit hard to understand how velocity is calculated as it is separated into multiple lines now. Maybe @dennisYatunin can take a look if this is ok?
|
Agree with the above. Separating the velocity computation into a couple of separate steps may lead to some errors. Would it be possible to keep the behavior of setting them all together while still switching to lazy broadcasts? Also a stupid question. Why do we need instead of
|
I suppose we could instead pass the state variable(s), and remove the cached variable and delay inlining of
That's not a stupid question-- it's because we aren't broadcasting over its arguments ( |
I think what @trontrytel meant here was |
Ah, sure, I think that will work. I kind of like the pointer assignment separate from the materialization, but it's not a big deal for now. And I'm happy to update to using |
charleskawczynski
force-pushed
the
ck/set_velocity_quantities
branch
from
96f2929 to
a63dcd2
Compare
|
I've updated to use the |
|
Also, I've opened #3613. I'll rebase this PR after that merges. |
charleskawczynski
force-pushed
the
ck/set_velocity_quantities
branch
3 times, most recently
from
6413832 to
4c3ed68
Compare
There was a problem hiding this comment.
This looks good to me now. @trontrytel @dennisYatunin Could you take a look again? And to make sure I understand, instead of passing ᶠuₕ³ around we are calculating it multiple times when calling set_velocity_quantities - is this something we want to do?
| ᶜuₕ = Y.c.uₕ | ||
| ᶠu₃ = Y.f.u₃ |
There was a problem hiding this comment.
I can remove it if it's not, I thought that I passed it to set_velocity_quantities!
There was a problem hiding this comment.
I did not see them used anywhere
| ᶜuₕ = Y.c.uₕ | ||
| ᶠu₃ = Y.f.u₃ |
Looks good to me also! Thank you. |
charleskawczynski
force-pushed
the
ck/set_velocity_quantities
branch
from
4c3ed68 to
685d858
Compare
WalkthroughThis change introduces two new functions, Possibly related PRs
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cache/precomputed_quantities.jl (1)
321-327: Add lazy broadcasting tocompute_ᶠu³The function is correctly implemented but missing the lazy broadcasting pattern used elsewhere in the code.
-compute_ᶠu³(ᶠuₕ³, ᶠu₃) = ᶠuₕ³ + CT3(ᶠu₃) +compute_ᶠu³(ᶠuₕ³, ᶠu₃) = @. lazy(ᶠuₕ³ + CT3(ᶠu₃))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cache/precomputed_quantities.jl(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: downstream ClimaCoupler.jl (1.10)
- GitHub Check: downstream ClimaCoupler.jl (1.11)
- GitHub Check: evaluate
- GitHub Check: docbuild
- GitHub Check: test (1.11)
- GitHub Check: test (1.10)
- GitHub Check: ci 1.11 - windows-latest
- GitHub Check: ci 1.11 - ubuntu-latest
- GitHub Check: ci 1.11 - macOS-latest
- GitHub Check: ci 1.10 - windows-latest
- GitHub Check: ci 1.10 - ubuntu-latest
- GitHub Check: ci 1.10 - macOS-latest
🔇 Additional comments (3)
src/cache/precomputed_quantities.jl (3)
313-319: Well-designed implementation ofcompute_ᶜuThe function is correctly implementing lazy broadcasting for cell-centered velocity computation, making the code more modular and reusable.
332-333: Good use of in-place broadcasting with computed valuesThe refactored code properly uses the new functions with
.=syntax, making the code more readable and consistent with Julia idioms.
470-470: Good conversion to in-place broadcastConverting to
.=syntax is appropriate for the lazy broadcasting approach.
|
I've dialed back some of the changes (as #3798 absorbed some suggestions). How do people feel about changing (; velocity_form) = p.atmos # velocity_form is a VelocityForm()
ᶜu .= compute_ᶜu(velocity_form, ᶜuₕ, ᶠu₃)
ᶠu³ .= compute_ᶠu³.(velocity_form, ᶠuₕ³, ᶠu₃)
ᶜK .= compute_kinetic(velocity_form, ᶜuₕ, ᶠu₃)? This way, we can ensure that all three of these implementations are linked, which I think was people's main concern? |
What is |
An instance of a (singleton) struct ( IIRC, people were concerned about getting rid of Using this type in all of the methods ( |
This PR is a step towards #3594.