MueLu: Fix filtered emin multigrid parameter hookup #13893
+84
−23
Conversation
This file contains 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
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
jhux2
added
the
AT: PRE-TEST INSPECTED
trilinos-autotester
removed
the
AT: PRE-TEST INSPECTED
|
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
Using Repos:
Pull Request Author: maxfirmbach |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
1 similar comment
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
jhux2
reviewed
Mar 19, 2025
There was a problem hiding this comment.
@maxfirmbach Is there a unit test that showed this error? If not, could you please add one?
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
Sure 👍 I will add one. |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
3 similar comments
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
trilinos-autotester
added
the
AT: STALE
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
|
@jhux2 I added a test case for elasticity on a stretched mesh with distance Laplacian dropping and emin, which now needs ~22 iterations, whereas before it would go over 40 with a way higher operator complexity. |
trilinos-autotester
removed
the
AT: STALE
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
jhux2
reviewed
Mar 25, 2025
There was a problem hiding this comment.
@maxfirmbach Why the change in how the nullspace is generated?
@jhux2 Good point. So only the coarse nullspace is actually used for the overall calculation, the fine one was commented out. Just using "Nullspace" makes the parameter hookup way easier avoiding duplicated factory calls. If there's a deeper reason behind giving both fine and coarse nullspace I can of course revert these changes. 👍 |
jhux2
previously approved these changes
Mar 25, 2025
|
I think I need another pre-merge inspection 😄. |
jhux2
added
the
AT: PRE-TEST INSPECTED
|
⚡ |
trilinos-autotester
removed
the
AT: PRE-TEST INSPECTED
|
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
Using Repos:
Pull Request Author: maxfirmbach |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
|
|
Two failing tests under 1 MPI rank4 MPI ranks |
|
I think it's fine to temporarily use |
|
@sebrowne Given the impending change of making AT2/cuda11 blocking, should we worry about the failure in this PR? The error message doesn't make me think that it's related to code changes. |
@jhux2 do you mean this one: ? That means that somebody internal needs to trigger the testing after having approved the PR. I show that the re-run that @cgcgcg triggered is running and building now. |
|
@sebrowne No, I meant this one: https://github.com/trilinos/Trilinos/actions/runs/14054936079/job/39352230200?pr=13893. [EDIT] But it's running now, so I guess it's ok? |
That's the follow-on from the one I posted. If it can't launch the container, it shows that it couldn't run a shell in the summary step. Annoying, but no way around it that we know about. Look further up at the "Set up runner" step and you'll see the first failure. |
|
@sebrowne Gotcha, thanks for the explanation! |
The emin multigrid parameter and factory hookup had a slight conceptual error. Changing the factory and parameter hookup to make things right and avoid duplicated factory calls. Signed-off-by: maxfirmbach <max@firmbach.com>
Signed-off-by: maxfirmbach <max@firmbach.com>
maxfirmbach
force-pushed
the
Fix-filtered-emin-multigrid
branch
from
80c86e2 to
287e713
Compare
cgcgcg
added
the
AT: PRE-TEST INSPECTED
cgcgcg
approved these changes
Mar 30, 2025
trilinos-autotester
removed
the
AT: PRE-TEST INSPECTED
|
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
Using Repos:
Pull Request Author: maxfirmbach |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
|
|
@maxfirmbach I think that relaxing the number of iterations just a bit should get the last test to pass. |
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.
@trilinos/muelu
Motivation
The emin multigrid parameter and factory hookup seems to have a slight conceptual error regarding matrix filtering. Thus, changing the factory and parameter hookup in case of a filtered matrix to make things right and avoid duplicated factory calls.
With the changes made the filtered matrix is now used for the prolongator pattern creation and the full operator for the actual minimization procedure (before it was the other way round).
@aprokop I was told you used this multigrid option at some point. Do you mind having a look at the changes? At least from a conceptual standpoint. Thanks! 😄
Might also be interesting for @mayrmt @alinaescher