Fix: UndefVarError in get_norm_matrix for BlockProjection#42
Open
Ady0333 wants to merge 2 commits intogridap:mainfrom
Open
Fix: UndefVarError in get_norm_matrix for BlockProjection#42Ady0333 wants to merge 2 commits intogridap:mainfrom
Ady0333 wants to merge 2 commits intogridap:mainfrom
Conversation
Replace undefined variable `s` with `a` and use BlockArray for proper Block indexing in return_cache(get_norm_matrix, ::BlockProjection). Signed-off-by: Ady0333 <adityashinde1525@gmail.com>
Contributor
Author
|
Hello @nichomueller , Please review this PR and let me know if any changes are required.... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #41
Problem
The
return_cachefunction forget_norm_matrix(::BlockProjection)insrc/RB/RBSteady/Projections.jl(lines 721-726) references an undefined variablesinstead of the actual parametera::BlockProjection.Broken code:
This is a copy-paste error from the adjacent
return_cachefunctions (lines 586-592) where the parameter is correctly nameds::BlockSnapshots. When adapting the function forBlockProjection, the parameter was renamed toa, but three references in the body (s.touched,ndims(s),size(s)) were not updated.Secondary issue: Even with the variable name fixed, the function allocated a plain
Arrayinstead of aBlockArray, which would fail onBlock(i,j)indexing downstream.Impact
Who is affected: Anyone running multi-field ROM workflows (Stokes, Navier-Stokes, coupled PDEs) that compute projection errors or assemble norm matrices.
What breaks:
get_norm_matrixon aBlockProjectioncrashes withUndefVarError: s not definedat line 722projection_erroronMultiFieldRBSpaceis completely brokenSilent failure? No, this is an unconditional hard crash. The feature is completely non-functional.
How to Reproduce
Confirmed with regression test:
Or via the standard multi-field workflow:
The crash happens unconditionally, every call to
get_norm_matrixon anyBlockProjectionhits this bug.Fix
Changed:
src/RB/RBSteady/Projections.jl, functionArrays.return_cache(::typeof(get_norm_matrix), a::BlockProjection)Replaced the broken 4-line body with a corrected 3-line implementation that:
a(not undefineds)num_fe_dofs(a)BlockArrayinstead of plainArrayFixed code:
This ensures:
a, nots)BlockArraywith proper block structure for multi-field indexingNet change: -1 line, but functionally complete rewrite of the body.
Testing
Added
test/RBSteady/block_projection_norm_matrix.jlwith regression tests that:BlockProjectionwith multiple blocksget_norm_matrix(bp)and verifies it doesn't crash withUndefVarErrorBlockArray(not plainArray)Test result before fix:
UndefVarError: s not definedat line 722Test result after fix: All assertions pass
After This Fix
get_norm_matrix(::BlockProjection)is fully functionalprojection_erroronMultiFieldRBSpacecompletes successfully