-
Notifications
You must be signed in to change notification settings - Fork 83
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
Tpetra LAI #1086
base: develop
Are you sure you want to change the base?
Tpetra LAI #1086
Conversation
e364a7e
to
2023fa6
Compare
2023fa6
to
252bdb3
Compare
252bdb3
to
92096dc
Compare
3ac5a34
to
a7e6c3a
Compare
a7e6c3a
to
a680573
Compare
On my Ubuntu machine, I am able to run both the two BTW, great PR @klevzoff! Thanks! |
Are you sure you compiled with
And the full log: ContactMechanics_SimpleCubes_08.data.txt I will run it in debug later when I get a chance and hopefully find out what that error actually means. |
Yes, I did a clean build with |
a680573
to
621c345
Compare
Turns out they pass for me too if I run each test separately, but fail when I run the whole test suite (and oversubscribe my cores). Not going to look into it anymore, it's just my setup that makes it fail. |
621c345
to
37944cd
Compare
I think this is as ready as it's going to be. Of the three failing tests that remain, two aren't expected to pass and one only has tiny diffs (see updated PR description). I've also updated Trilinos to 13.0.0 released a couple weeks ago and it seems to be running fine. Before getting this PR merged, I need someone to build the updated TPLs on LC (and preferably test the build of GEOSX PR as well) and push updated host-configs. |
dab4dcc
to
6b68f4c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done! Thanks for removing some add
, insert
, set
combinations no longer used.
src/coreComponents/linearAlgebra/interfaces/hypre/HyprePreconditioner.cpp
Show resolved
Hide resolved
{ | ||
// Default options only for the moment | ||
GEOSX_LAI_CHECK_ERROR( PCSetType( precond, PCGAMG ) ); | ||
GEOSX_UNUSED_VAR( params ) | ||
|
||
// TODO: need someone familiar with PETSc to take a look at this | ||
#if 0 | ||
GEOSX_LAI_CHECK_ERROR( PCSetType( prec, PCHMG ) ); | ||
GEOSX_LAI_CHECK_ERROR( PCHMGSetInnerPCType( prec, PCGAMG ) ); | ||
GEOSX_LAI_CHECK_ERROR( PCSetType( precond, PCHMG ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this PR ... but are we still planning to improve this section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be great, but I don't have a plan. @castelletto1 has spent quite some time trying to make it work and looks like it's not at all straightforward.
bool const keepDiag, | ||
real64 const diagValue ) | ||
{ | ||
// TODO: Deprecate/remove this method? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this method still used to impose BC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, since BCs are now imposed on the local matrix/rhs objects prior to creation of parallel matrix/rhs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so it can be safely removed.
599d972
to
319bd00
Compare
319bd00
to
58a47e3
Compare
This PR adds support for Tpetra-based LAI. It provides similar functionality to other existing LA interfaces. It is not enabled by default, meaning the Epetra-based Trilinos interface is still default. The plan is to hopefully get it used and tested and eventually replace the old one.
It uses OpenMP and/or GPU acceleration that is available (via Kokkos) in Trilinos packages:
There is still a host-based matrix copy involved - the issue of copy avoiding is a lot more complex and is not addressed here. For vectors, the copy from
LvArray::ArrayView
occurs on device, however there is another host copy that gets allocated to support Tpetra's dual-view semantics. These cannot currently be eliminated due to a fundamental issue - Tpetra requires all its device allocations to be in Unified Memory (most solvers, in particular Krylov and preconditioners, seem to work fine when given non-UM vector views, but direct solver KLU2 fails). Some issues in their tracker dating back 4 years suggest relaxing that requirement may not be easy for them any time soon.More specifically:
TpetraVector
,TpetraMatrix
,TrilinosTpetraPreconditioner
,TrilinosTpetraSolver
andTrilinosTpetraInterface
, with identical capabilities to exising Trilinos/Hypre/Petsc classespreconditionerType
input - it was mainly useful to troubleshootBlockPreconditioner
(eliminating the possibility of sub-block solvers underperforming) and I decided to leave it inThe new interface:
Integrated tests that fail:
Kokkos::Impl::ParallelReduce< Cuda > requested too much L0 scratch memory
during MueLu prolongation construction, new (did not fail earlier, including post-13.0 Trilinos update)I also need someone to test the PR (including TPL build) on Lassen for possible build issues, configuring with
-D GEOSX_LA_INTERFACE=TrilinosTpetra
. Other than that, this is ready to merge, rebaseline not required.Related to:
GEOS-DEV/thirdPartyLibs#120
GEOS-DEV/LvArray#189
GEOS-DEV/GEOSX_PTP#20
https://github.com/GEOSX/integratedTests/pull/84