Update defaults for distributed multigrid #1612
Conversation
core/solver/multigrid.cpp
Outdated
| #if GINKGO_BUILD_MPI | ||
| if (gko::detail::is_distributed(matrix.get())) { | ||
| using absolute_value_type = remove_complex<value_type>; | ||
| return solver::Gmres<value_type>::build() |
There was a problem hiding this comment.
this is quite similar/identical to the case for the dpcpp executor, maybe it could be extracted.
|
@pratikvn I will fix the CI issues to push this for the release. |
15394ed
to
1fefb41
Compare
| MultigridState() : nrhs{0} {} | ||
| MultigridState() : nrhs{static_cast<size_type>(-1)} {} |
There was a problem hiding this comment.
no, because then it will not initialize the cache, if the rhs has 0 columns.
There was a problem hiding this comment.
would there be a downside to allow initializing the cache to a nx0 vector?
There was a problem hiding this comment.
the distributed solver test uses cases with zero columns and the MG preconditioned CG passes those, so it seems to work as expected.
| MultigridState() : nrhs{0} {} | ||
| MultigridState() : nrhs{static_cast<size_type>(-1)} {} |
There was a problem hiding this comment.
would there be a downside to allow initializing the cache to a nx0 vector?
|
One thing to note is that the added tests are EXTREMELY slow. On nla-gpu the preconditioned test are 100x slower than the unpreconditioned tests. But I don't think this PR should fix performance issues, so I will just leave it as it is. |
|
slow as in long runtime or as in slow convergence (or both)? In any case, I think we should reconsider whether to merge this PR in this state then |
definitely runtime, but I think the number of iterations is also quite bad. IMO this PR should still continue, since otherwise the user will get segfaults and the like. Even sequentially our mg is not very good, so it wouldn't be much of a difference to that. |
|
The slow convergence is expected in the 3pt stencil because the PGM does not handle the equal-weight matrix well currently. |
|
Alright, then I withdraw my objection ;) that's an issue we should definitely tackle soon though. I think with some deterministic or randomized tie-breaking we should be able to handle that, create at least a 25%-33% reduction per level (haven't thought the worst case through entirely) |
|
Given that we are testing that the solver + preconditioner work, I think we should just set the factory parameter |
Can't confirm that this has any effect (on nla-gpu). |
45a5e93
to
4b1dcfa
Compare
since rocm 4.5 doesn't handle SPGEAM with empty matrices correctly, the test case with some (but not all) empty partitions has to be skipped.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1612 +/- ##
===========================================
- Coverage 90.01% 89.97% -0.04%
===========================================
Files 736 752 +16
Lines 59831 60445 +614
===========================================
+ Hits 53857 54387 +530
- Misses 5974 6058 +84 ☔ View full report in Codecov by Sentry. |
This PR updates the defaults for distributed multigrid for the smoother and the coarse solver. It also adds a test for the distributed pgm preconditioned solver