Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #103 +/- ##
==========================================
- Coverage 66.56% 66.55% -0.02%
==========================================
Files 62 62
Lines 4065 4072 +7
==========================================
+ Hits 2706 2710 +4
- Misses 1359 1362 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR appears to be a cross-cutting bugfix/cleanup sweep across GridapSolvers’ solver interfaces and multilevel tooling, correcting several clear typos/variable mixups and fixing logic that previously evaluated both branches (via ifelse) in continuation operators.
Changes:
- Fixes multiple variable/field-reference mistakes and tuple destructuring errors that would cause runtime failures.
- Replaces
ifelsewithifblocks inContinuationFEOperatorto avoid evaluating both branches. - Adjusts several solver/operator internals (patch solver caches ordering, Richardson iteration caches, Schur complement intermediate usage) for correctness.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/SolverInterfaces/PAExtras.jl |
Fix CSR matrix row-count field used when constructing a view. |
src/SolverInterfaces/NullSpaces.jl |
Adjusts orthogonality check for matrix input (but introduces a dimension assertion bug). |
src/PatchBasedSmoothers/PatchSolvers.jl |
Fixes wrong variable reference (nc→ns) and returns patch axes in consistent order. |
src/NonlinearSolvers/ContinuationFEOperators.jl |
Avoids eager evaluation via ifelse in residual/jacobian allocation and application (but jacobian! branch for reuse_caches=false is currently broken). |
src/MultilevelTools/ModelHierarchies.jl |
Fixes tuple assignment arity (nothing, nothing, nothing → nothing, nothing). |
src/MultilevelTools/HierarchicalArrays.jl |
Fixes map! to actually write into the destination hierarchical array. |
src/MultilevelTools/GridTransferOperators.jl |
Fixes tuple assignment arity in redistribution operator construction. |
src/LinearSolvers/SchurComplementSolvers.jl |
Fixes use of the correct intermediate (x_u) when forming the Schur RHS. |
src/LinearSolvers/RichardsonLinearSolvers.jl |
Simplifies caches by removing redundant α vector; uses ω directly. |
src/BlockSolvers/BlockFEOperators.jl |
Fixes wrong variable name in constructor and corrects linear-form application signature. |
src/BlockSolvers/BlockDiagonalSolvers.jl |
Fixes mapping arity when symbolic setup includes an initial guess x. |
ext/GridapPETScExt/HipmairXuSolvers.jl |
Fixes missing D definition used in VectorValue{D,...} FE construction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| jacobian(op.op1, u) | ||
| else | ||
| jacobian(op.op2, u) |
There was a problem hiding this comment.
Algebra.jacobian!(A, op::ContinuationFEOperator, u) ignores the provided matrix A when reuse_caches is false and calls the allocating jacobian(op.op*, u) without copying the result into A. If callers invoke jacobian! for its side effects (common for bang methods), the computed Jacobian can be dropped, and the method also violates the expectation that jacobian! updates A/returns it. Please change this branch to mutate A (e.g., call jacobian!(A, op.op*, u) consistently, or explicitly copy the result into A), and consider adding test coverage for reuse_caches=false since current tests appear to exercise only reuse_caches=true.
| jacobian(op.op1, u) | |
| else | |
| jacobian(op.op2, u) | |
| jacobian!(A, op.op1, u) | |
| else | |
| jacobian!(A, op.op2, u) |
There was a problem hiding this comment.
When reuse_caches is false, we do not want to compute the jacobian in place. This option is meant to be used when the sparsity pattern of the matrix changes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
No description provided.