Conversation
Moved MUMPS solver structure declaration below stencils for proper initialization.
Reordered MUMPS solver structure declaration to follow stencils.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #182 +/- ##
==========================================
- Coverage 95.18% 95.14% -0.04%
==========================================
Files 94 89 -5
Lines 9345 8925 -420
==========================================
- Hits 8895 8492 -403
+ Misses 450 433 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| : DirectSolver(grid, level_cache, domain_geometry, density_profile_coefficients, DirBC_Interior, num_omp_threads) | ||
| { | ||
| solver_matrix_ = buildSolverMatrix(); | ||
| initializeMumpsSolver(mumps_solver_, solver_matrix_); | ||
| SparseMatrixCOO<double> solver_matrix = buildSolverMatrix(); | ||
| mumps_solver_.emplace(std::move(solver_matrix)); |
There was a problem hiding this comment.
Why can't you just do:
: DirectSolver(grid, level_cache, domain_geometry, density_profile_coefficients, DirBC_Interior, num_omp_threads)
, mumps_solver_(buildSolverMatrix())
{}
?
This would also avoid the need for std::optional
There was a problem hiding this comment.
I tried this but doesn't work really for 2 reasons.
-
In the Smoother we have a
void buildMatrix(inner&, tridiags&) -
If we do this, then we need to do the Stencil assignments before the definition of the Matrices in the private: section. Otherwise Stencils are not yet defined when we run buildMatrix.
There was a problem hiding this comment.
Do we have to do the same for all classes?
What makes sense for the Smoother isn't necessarily relevant for the solver is it?
In the Smoother we have a
void buildMatrix(inner&, tridiags&)
I can't find the function you are talking about. I just see buildAscMatrices which seem like functions which could become:
auto SmootherGive::buildAscMatrices()
or
MatrixType SmootherGive::buildAscMatrices()
There was a problem hiding this comment.
Yes, I was referring to that function. These modify the private inner boundary matrix as well as the tridiag matrices.
It would work for the DirectSolver, however it would be nice if we keep it consistent with the Smoother as well.
|
|
||
| #ifdef GMGPOLAR_USE_MUMPS | ||
| initializeMumpsSolver(inner_boundary_mumps_solver_, inner_boundary_circle_matrix_); | ||
| inner_boundary_mumps_solver_.emplace(std::move(inner_boundary_circle_matrix_)); |
There was a problem hiding this comment.
Is it safe to use std::move here? inner_boundary_circle_matrix_ is not used when MUMPS is activated?
There was a problem hiding this comment.
inner_boundary matrix will never be used again after this, so it would be fine. But let me know if you prefer a copy.
The COO Solver takes ownership of A while the CSR Solver doesn't.
There was a problem hiding this comment.
I don't like the fact that a class member is uninitialised and must not be used. I would prefer if inner_boundary_mumps_solver_ and inner_boundary_circle_matrix_ could be condensed into 1 variable to avoid the risk of accidentally trying to access inner_boundary_circle_matrix_
There was a problem hiding this comment.
I don't think it makes sense to combine them into one variable.
I will just remove the std::move.
In the DirectSolver there are no further changes needed.
Merge Request - GuideLine Checklist
Guideline to check code before resolve WIP and approval, respectively.
As many checkboxes as possible should be ticked.
Checks by code author:
Always to be checked:
If functions were changed or functionality was added:
If new functionality was added:
If new third party software is used:
If new mathematical methods or epidemiological terms are used:
Checks by code reviewer(s):