Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linear algebra cleanup #1363

Merged
merged 3 commits into from
Apr 14, 2021
Merged

Linear algebra cleanup #1363

merged 3 commits into from
Apr 14, 2021

Conversation

klevzoff
Copy link
Contributor

@klevzoff klevzoff commented Mar 19, 2021

Not sure how to best describe this PR. Essentially I'm splitting off a bunch of code cleanup/refactoring from my multiscale branch to get reviewed separately. I'm not sure how it came to include 127 files, but I don't think I touched any files unnecessarily.

Most notably:

  1. Refactor direct solver interfaces (SuiteSparse and SuperLU_Dist) to reduce code duplication:
    • Each one is taken out of package-specific solver classes and made into a full-fledged solver class of its own, templated on LAI.
    • Differences between 3 LA packages in how matrix/vector entries are extracted and/or gathered/scattered (in case of SuiteSparse) are abstracted by new set of classes, HypreExport/PetscExport/TrilinosExport.
  2. Add a new base class, LinearSolverBase, that extends the concept of PreconditionerBase and is a new common base for new direct solver classes and package-specific solver classes (HypreSolver, etc.). The latter now only implement iterative solves and have been split into setup/solve stages, which allows using them as preconditioners (in case someone wants a really accurate inverse approximation, for comparison purposes).
  3. Simplify PreconditionerBase interface by removing the overload setup(Matrix const &, DofManager const &). Instead, added the ability to "attach" a DofManager to a matrix object (essentially, just store a pointer to it with the matrix). This makes a whole lot of things a whole lot simper.
  4. Reimplement a few operations in HypreMatrix/HypreVector replacing raw loops with RAJA kernels and correct exec policy. This should make them usable with hypre-GPU. Not all operations are fully GPU-safe; some, like single-row-access stuff, don't make sense on the GPU and should potentially be deprecated/removed.
  5. Split testLAOperations into separate CTest targets for vectors, matrices and solvers (now prefixed with testLinearAlgebra, for easier grouping). Implemented a proper unit test for vector classes that tests each function in vector's API (actually this is brought over from my unmerged Tpetra PR) - we should do this for matrices at some point but it's way more tedious.
  6. Reorganized some file locations and/or names (e.g. GMRESsolver -> GmresSolver to be consistent with project conventions).
  7. Cleaned up Hypre MGR stuff a bit by splitting into separate recipe classes. Not yet the final form I'd like to see, but hopefully a small step towards a more declarative style description of recipes.

The rest is just code cleanup and minor usability improvements here and there.

All tests pass, no rebaseline necessary rebaseline needed because some linear solver parameters changed type from strings to enums and some changed enum values (we should really be adding new options at the end, to avoid value changes, but I took this chance since I'll be rebaselining anyway).

Rebaseline PR: https://github.com/GEOSX/integratedTests/pull/130

@klevzoff klevzoff added the type: cleanup / refactor Non-functional change (NFC) label Mar 19, 2021
@klevzoff klevzoff self-assigned this Mar 19, 2021
@klevzoff klevzoff force-pushed the feature/klevzoff/lai-cleanup branch 2 times, most recently from 541cb86 to 3362dc7 Compare March 29, 2021 07:04
@klevzoff klevzoff changed the title LAI cleanup Linear algebra cleanup Mar 29, 2021
@klevzoff klevzoff force-pushed the feature/klevzoff/lai-cleanup branch from 3362dc7 to 3f6b339 Compare March 30, 2021 00:25
@klevzoff klevzoff force-pushed the feature/klevzoff/lai-cleanup branch from 3f6b339 to e65011c Compare April 1, 2021 09:32
@klevzoff klevzoff force-pushed the feature/klevzoff/lai-cleanup branch 3 times, most recently from 0d0ee60 to e9353ae Compare April 3, 2021 20:55
@klevzoff klevzoff added the flag: requires rebaseline Requires rebaseline branch in integratedTests label Apr 3, 2021
* 3. C-points coarse-grid/Schur complement solver: boomer AMG
* 4. Global smoother: none
*/
class SinglePhasePoroelastic : public MGRStrategyBase< 1 >
Copy link
Contributor Author

@klevzoff klevzoff Apr 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@castelletto1 @francoishamon I refactored MGR recipes into these classes. The class constructor chooses all relevant MGR parameter values, and then the setup function applies them on the solver. Ideally I wanted the recipes to be purely static sets of parameter choices which then get applied by some generic common code, but some dynamism is necessary (in particular, to set up coarse indices in variable-size problems like multiphase; but also maybe some parameter choices could depend on runtime characteristics of the problem, like size?), so I left it like this. We can feed more data about the problem into the constructor as we need it.

Anyway, I really hope that I didn't mess up when transferring the parameter values. I'm running a CO2 tutorial and the two multiphase benchmarks in the repo that use MGR, but for other solvers I will have to just switch some of the integrated tests to MGR to verify. But even then I don't know what optimal performance looked like. Let me know if you see anything weird here or if you have other cases you'd like me to run.

Copy link
Contributor Author

@klevzoff klevzoff Apr 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a side note, it baffles me that hypre does not provide named constants for various parameter choices, at least in the form of macro defs, and keeps using raw integer constants - even in its central, most established components like BoomerAMG. If you're lucky, you'll find a description in function comment, but I found those to be missing or out-of-date sometimes.

If hypre has a fundamental/philosophical reason to not provide constants, do you think it would be worthwhile defining them on the GEOSX side? E.g.

namespace geosx { 
namespace hypre {
enum RelaxType : HYPRE_Int
{
  jacobi = 0, // Jacobi
  seqGaussSeidel = 1, // Gauss-Seidel, sequential (very slow!)
  ...
  l1jacobi = 18 // \f$\ell_1\f$-scaled jacobi
};
} }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I would suggest to have separate files for each MGR strategy class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd much clearer if we introduced (not necessarily in this PR) constants in GEOSX as you're suggesting

@klevzoff klevzoff force-pushed the feature/klevzoff/lai-cleanup branch from e9353ae to ad5ecb1 Compare April 3, 2021 22:41
Copy link
Contributor

@andrea-franceschini andrea-franceschini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job! Thanks

@klevzoff klevzoff force-pushed the feature/klevzoff/lai-cleanup branch from 4f7221e to 51f8019 Compare April 7, 2021 06:39
Copy link
Contributor

@castelletto1 castelletto1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linear algebra component keeps improving, thank you very much @klevzoff! Comments I left don't need to be all addressed within this PR. Some could definitely be deferred for a Doc Day. IMO time is mature to put together an rst document giving an overview of linear algebra in GEOSX (beyond linear solvers).

Comment on lines +20 to +22
#include "linearAlgebra/solvers/BicgstabSolver.hpp"
#include "linearAlgebra/solvers/CgSolver.hpp"
#include "linearAlgebra/solvers/GmresSolver.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the capitalized version (BICGSTABSolver, CGSolver, GMRESSolver) since these are all acronyms. This follows the same naming logic adopted in physics solvers where FEM, FVM -- and not Fem, Fvm -- are appended. If we wanted to be very sophisticated we may use BiCGStabSolver and GMResSolver, which I don't like though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be on board if we found a way to have the acronym at the end of the name as in LaplaceFEM and SinglePhaseFVM. Having another capital letter follow the uppercase acronym feels a bit off to me, nor do I like the old scheme (CGsolver). Maybe SolverCG (not too pretty either, but feels tolerable)?

Comment on lines 62 to 72
// WARNING. We don't have consistent types between HYPRE_Int and localIndex.
// Decision needs to be made either to use bigint option, or change
// localIndex to int. We are getting away with this because we do not
// pass ( localIndex * ) to hypre except when it is on the GPU, in
// which case we are using int for localIndex.
#if defined(GEOSX_USE_HYPRE_CUDA)
static_assert( sizeof( HYPRE_Int ) == sizeof( localIndex ),
"HYPRE_Int and geosx::localIndex must have the same size" );
#endif
Copy link
Contributor

@castelletto1 castelletto1 Apr 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what actions were finally decided with the hypre team on this issue. @rrsettgast any recent updates?

@castelletto1
Copy link
Contributor

@castelletto1
Copy link
Contributor

@klevzoff
Copy link
Contributor Author

Could we move this

https://github.com/GEOSX/GEOSX/blob/51f80196beb7fc726c911fa47e0c04de8d1b6d7b/src/coreComponents/linearAlgebra/interfaces/InterfaceTypes.hpp#L36-L40

somewhere in codingUtilities?

Yeah. I think common/GeosxMacros.hpp will do for now.

@klevzoff
Copy link
Contributor Author

Until we actually need them, I suggest to simplify the interface to:

static void initialize( ); 

Done (after push)

@klevzoff klevzoff force-pushed the feature/klevzoff/lai-cleanup branch from 04a612e to 87c5283 Compare April 13, 2021 20:42
@klevzoff klevzoff force-pushed the feature/klevzoff/lai-cleanup branch from 87c5283 to a75948b Compare April 14, 2021 00:05
@klevzoff klevzoff added ci: run CUDA builds Allows to triggers (costly) CUDA jobs and removed flag: ready for review labels Apr 14, 2021
@klevzoff klevzoff removed flag: requires rebaseline Requires rebaseline branch in integratedTests flag: requires updated submodule(s) labels Apr 14, 2021
@klevzoff klevzoff merged commit 3da629e into develop Apr 14, 2021
@klevzoff klevzoff deleted the feature/klevzoff/lai-cleanup branch April 14, 2021 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci: run CUDA builds Allows to triggers (costly) CUDA jobs type: cleanup / refactor Non-functional change (NFC)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants