Skip to content

add hyperdiffusion of area fraction#4423

Draft
szy21 wants to merge 5 commits intomainfrom
zs/area_hyperdiff
Draft

add hyperdiffusion of area fraction#4423
szy21 wants to merge 5 commits intomainfrom
zs/area_hyperdiff

Conversation

@szy21
Copy link
Copy Markdown
Member

@szy21 szy21 commented Apr 9, 2026

Purpose

To-do

Content


  • I have read and checked the items on the review checklist.

@szy21 szy21 force-pushed the zs/area_hyperdiff branch 2 times, most recently from 77c2cae to 637e5d0 Compare April 14, 2026 04:29
@szy21 szy21 marked this pull request as draft April 14, 2026 04:29
@costachris costachris requested a review from Copilot April 17, 2026 23:08
@szy21
Copy link
Copy Markdown
Member Author

szy21 commented Apr 17, 2026

@copilot review this PR

Copy link
Copy Markdown

Copilot AI commented Apr 17, 2026

@copilot review this PR

Here's a review of the two commits in this PR:

Commit 1 – "add hyperdiffusion of area fraction"

  • Adds ᶜ∇²aʲs (Laplacian of area fraction) to the hyperdiffusion cache, computes it via wdivₕ(gradₕ(draft_area(ρa, ρʲ))), and registers it in the DSS pairs. The approach mirrors the existing ᶜ∇²mseʲs pattern, which is clean.
  • ᶜρʲs is correctly pulled from p.precomputed in prep_hyperdiffusion_tendency! to compute the area fraction a = ρa/ρʲ.

Commit 2 – "no hyperdiffusion on area and updraft velocity"

This commit introduces a few issues:

  1. Dead computationᶜ∇²aʲs is now computed (including DSS) but never applied. Allocating, computing, and communicating a field that is never used wastes memory and compute. If the tendency is intentionally disabled, the computation in prep_hyperdiffusion_tendency! and the DSS pair in dss_hyperdiffusion_tendency_pairs should also be removed or guarded.

  2. ᶜρʲs pulled unnecessarily in apply_hyperdiffusion_tendency! – the only use of ᶜρʲs in that function is the commented-out line. The destructuring (; ᶜρʲs) = p.precomputed at line 152 can be dropped until the tendency is actually applied.

  3. Disabling existing updraft velocity hyperdiffusion – the line

    @. Yₜ.f.sgsʲs.:($$j).u₃ += ν₄_vorticity * ᶠwinterp(ᶜJ * ᶜρ, ᶜ∇⁴uᵥʲ)

    was active before this PR and is now commented out. This is a behaviour change for the updraft velocity that is not mentioned in the PR description. Is this intentional? If so, it deserves an explanation.

  4. Commented-out code without context – the two # @. Yₜ... lines give no hint of why they are disabled. A brief comment (e.g. # disabled pending stability testing) would help future readers.

Suggestions:

  • Either enable the area fraction tendency (and decide on the form – with/without ᶜρʲs weighting, matching the pattern of ᶜ∇²mseʲs) or remove the ᶜ∇²aʲs allocation/computation/DSS entirely until it is ready to use.
  • Document the reason for disabling updraft velocity hyperdiffusion, or revert that change if it was accidental.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to add hyperdiffusion support for the EDMFX subdomain area fraction (via ρa / a-related operators) within the existing hyperdiffusion tendency pipeline.

Changes:

  • Extend the hyperdiffusion cache with ᶜ∇²aʲs for EDMFX subdomains.
  • Compute ᶜ∇²aʲs during prep_hyperdiffusion_tendency! and include it in DSS synchronization pairs.
  • Modify SGS hyperdiffusion application code paths (including commenting out an existing u₃ update).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@. Yₜ.f.sgsʲs.:($$j).u₃ += ν₄_vorticity * ᶠwinterp(ᶜJ * ᶜρ, ᶜ∇⁴uᵥʲ)
# @. Yₜ.f.sgsʲs.:($$j).u₃ += ν₄_vorticity * ᶠwinterp(ᶜJ * ᶜρ, ᶜ∇⁴uᵥʲ)
end
# @. Yₜ.c.sgsʲs.:($$j).ρa -= ν₄_scalar * wdivₕ(ᶜρʲs.:($$j) * gradₕ(ᶜ∇²aʲs.:($$j)))
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ᶜ∇²aʲs is computed and added to the DSS pairs, but the only place it would affect the solution (the Yₜ.c.sgsʲs[j].ρa hyperdiffusion update) is currently commented out. As written, this adds allocation/DSS overhead without changing model behavior; either enable the tendency update for ρa or remove the cache/DSS plumbing for ᶜ∇²aʲs until it is actually used.

Suggested change
# @. Yₜ.c.sgsʲs.:($$j).ρa -= ν₄_scalar * wdivₕ(ᶜρʲs.:($$j) * gradₕ(ᶜ∇²aʲs.:($$j)))
@. Yₜ.c.sgsʲs.:($$j).ρa -= ν₄_scalar * wdivₕ(ᶜρʲs.:($$j) * gradₕ(ᶜ∇²aʲs.:($$j)))

Copilot uses AI. Check for mistakes.
# only need curl-curl part
ᶜ∇⁴uᵥʲ = @. ᶜ∇²uᵥʲs.:($$j) = C3(wcurlₕ(C123(curlₕ(ᶜ∇²uʲs.:($$j)))))
@. Yₜ.f.sgsʲs.:($$j).u₃ += ν₄_vorticity * ᶠwinterp(ᶜJ * ᶜρ, ᶜ∇⁴uᵥʲ)
# @. Yₜ.f.sgsʲs.:($$j).u₃ += ν₄_vorticity * ᶠwinterp(ᶜJ * ᶜρ, ᶜ∇⁴uᵥʲ)
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change comments out the SGS vertical-velocity hyperdiffusion update (Yₜ.f.sgsʲs[j].u₃ ...) in the 3D-point branch. If this wasn’t intentional, it will remove an existing dissipation mechanism and likely change stability/solution behavior; please restore the update (or replace it with the corrected form) rather than leaving it commented out.

Suggested change
# @. Yₜ.f.sgsʲs.:($$j).u₃ += ν₄_vorticity * ᶠwinterp(ᶜJ * ᶜρ, ᶜ∇⁴uᵥʲ)
@. Yₜ.f.sgsʲs.:($$j).u₃ += ν₄_vorticity * ᶠwinterp(ᶜJ * ᶜρ, ᶜ∇⁴uᵥʲ)

Copilot uses AI. Check for mistakes.
@szy21 szy21 force-pushed the zs/area_hyperdiff branch 5 times, most recently from 866d532 to cc77fd7 Compare April 24, 2026 17:51
@szy21 szy21 force-pushed the zs/area_hyperdiff branch 4 times, most recently from 497c3fd to 650c8a6 Compare April 24, 2026 21:03
@szy21 szy21 force-pushed the zs/area_hyperdiff branch from 650c8a6 to 3b73dad Compare April 25, 2026 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants