Fix numerical Schlieren for single fluid IGR + add case validation guard for schlieren_alpha#1477
Fix numerical Schlieren for single fluid IGR + add case validation guard for schlieren_alpha#1477Cowsreal wants to merge 5 commits into
Conversation
sbryngelson
left a comment
sbryngelson
left a comment
There was a problem hiding this comment.
Careful review — this is a clean, correct bug fix. I verified the core logic:
- Index math (
m_derived_variables.fpp:501–513): for IGR,eqn_idx%adv%end − eqn_idx%E = num_fluids − 1, so the loop coversschlieren_alpha(1..N−1)and the new term correctly appliesschlieren_alpha(num_fluids)to the reconstructedalpha_last = 1 − Σαᵢ. This reproduces exactly what the non-IGR branch computes. - The
if (igr)term lives inside themodel_eqns /= 1branch and IGR only exists undermodel_eqns == 2, so non-IGR paths are untouched. - Validator default
schlieren_alpha = dflt_real(−1e6) confirmed, so the new "must be set" guard is well justified, and theif/elserestructure preserves both original prohibits. - No regressions: all example cases touching schlieren (
2D_IGR_triple_point,2D_triple_point) set everyschlieren_alpha; no golden/test cases generate schlieren.
Only minor, non-blocking notes inline (a comment typo, a loop-invariant branch, and an edge-config suggestion). LGTM once the typo is fixed.
| end do | ||
|
|
||
| ! IGR stores only num_fluids-1 volume fractions; the last fluid's volume fraction is untracked. | ||
| ! For a single fluid this is simply 1.0 everywhere. Without this term, the entire single-fluid |
There was a problem hiding this comment.
Nit: stray closing paren in "the entire single-fluid Schlieren field)" — should be "the entire single-fluid Schlieren field".
| ! IGR stores only num_fluids-1 volume fractions; the last fluid's volume fraction is untracked. | ||
| ! For a single fluid this is simply 1.0 everywhere. Without this term, the entire single-fluid | ||
| ! Schlieren field) is dropped, leaving exp(0) = 1 everywhere. Compute that term below. | ||
| if (igr) then |
There was a problem hiding this comment.
Optional (efficiency/clarity): both if (igr) checks (here and the one inside the i loop at line 504) test a loop-invariant flag evaluated on every (j,k,l) cell. Since igr is constant for the whole run, you could hoist the IGR/non-IGR split outside the triple loop — e.g. keep the inner alpha_last accumulation only when igr, or branch once above the loops. Negligible cost in post-processing, so not blocking, just a readability note.
Also note this new term divides by gm_rho_max(1), which is 0 for a perfectly uniform density field — the same pre-existing risk as line 502, so no regression, just flagging.
| if schlieren_alpha is not None: | ||
| if schlieren_alpha is None: | ||
| self.prohibit( | ||
| schlieren_wrt and model_eqns is not None and model_eqns != 1, |
There was a problem hiding this comment.
The guard correctly excludes model_eqns == 1, which matches the Fortran (the model_eqns == 1 branch never uses schlieren_alpha). One edge case to consider: IGR's adv-index layout is only set up under model_eqns == 2 (m_global_parameters.fpp), but nothing forbids igr = T with model_eqns ∈ {3,4}. In that (unsupported) combo the new if (igr) term in m_derived_variables.fpp would double-count the last fluid. IGR is fundamentally a model_eqns == 2 feature and would likely fail elsewhere, so this is theoretical — but a belt-and-suspenders self.prohibit(igr and model_eqns is not None and model_eqns != 2, ...) in check_igr would make the assumption explicit.
Minor: if model_eqns is unset (None), the guard won't fire even with schlieren_wrt on; fine in practice since model_eqns is virtually always specified.
There was a problem hiding this comment.
Irrelevant, IGR is blocked to run for model_eqn /= 2
sbryngelson
left a comment
sbryngelson
left a comment
There was a problem hiding this comment.
Requesting changes to track the follow-ups from the inline review. The core fix is correct (IGR index math reproduces the non-IGR result exactly, and the validator default dflt_real justifies the new guard), so these are small:
Please address before merge:
- Fix the comment typo at
m_derived_variables.fpp:510— stray closing paren in "Schlieren field)".
Optional (fine to defer or wave off):
2. Hoist the loop-invariant if (igr) branch out of the (j,k,l) triple loop in m_derived_variables.fpp (readability only).
3. Consider an explicit igr ⇒ model_eqns == 2 guard in check_igr so the new if (igr) Schlieren term can't double-count under an unsupported model_eqns ∈ {3,4} config.
See inline comments for details. Happy to approve once (1) is in.
|
Reviewed this carefully — nice catch on the single-fluid IGR Schlieren bug. The fix is correct: under IGR the loop covers I've left a few small inline notes (one comment typo to fix, plus a couple of optional hardening/readability suggestions) and marked the review as Request Changes just to track the typo. Everything else is non-blocking — happy to approve once that's addressed. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1477 +/- ##
==========================================
- Coverage 60.64% 60.63% -0.02%
==========================================
Files 73 73
Lines 20213 20219 +6
Branches 2936 2937 +1
==========================================
Hits 12259 12259
- Misses 5966 5972 +6
Partials 1988 1988 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Description
For N fluids, IGR tracks the volume fraction of the first N-1 fluids. The part of the code in m_derived_variables.fpp in post-processing did not account for this and writes out a field of 1.0's instead.
A guard for schlieren_alpha is also necessary since schlieren_alpha(i) is set to dflt_real and the same result is seen if that's the case.
Type of change
Testing
After fix: (IGR, single fluid)
Checklist
AI code reviews
Reviews are not triggered automatically. To request a review, comment on the PR:
@coderabbitai review— incremental review (new changes only)@coderabbitai full review— full review from scratch/review— Qodo review/improve— Qodo code suggestions@claude full review— Claude full review (also triggers on PR open/reopen/ready)claude-full-review— Claude full review via label