Ifpack2: Hypre interface and ordinal type matching

[berselius]

@trilinos/ifpack2

Motivation

• I want to use the hypre interface for BoomerAMG preconditioners, which we previously used on the Epetra stack, for Tpetra conversion acceptance
• See Ifpack2: HYPRE interface #15085
• This interface tested, and it seems that the Tpetra global_ordinal and hypre's quasi-equivalent HYPRE_BigInt did not map to each other in the Ifpack2 hypre interface implementation

Testing

• I was able to use these changes in Trilinos 16.2 to successfully call BoomerAMG and Hypre_PCG in my tpetra-based application code.

[github-actions]

CDash for AT2 results [Currently only accessible from Sandia networks]

[cgcgcg]

Did Hypre change this? Because originally this definitely did build and run correctly. Maybe use a typedef so that it's easier to change it back?

[berselius]

I wonder if you built hypre/trilinos in a different way - I think the way the code is currently set up would work if local_ordinal and global_ordinal were both int, which is the default out of the box with hypre spack builds

[github-actions]

Your PR updated files that did not respect package formatting settings.
Please apply the patch given below. Alternatively you can download a patch file here.

Patch

diff --git a/packages/ifpack2/src/Ifpack2_Hypre_decl.hpp b/packages/ifpack2/src/Ifpack2_Hypre_decl.hpp
index e21d1476..4e46d6b8 100644
--- a/packages/ifpack2/src/Ifpack2_Hypre_decl.hpp
+++ b/packages/ifpack2/src/Ifpack2_Hypre_decl.hpp
@@ -287,13 +287,13 @@ class Hypre : virtual public Ifpack2::Preconditioner<typename MatrixType::scalar
 
 template <class LocalOrdinal, class Node>
 class Hypre<Tpetra::RowMatrix<HYPRE_Real, LocalOrdinal, HYPRE_BigInt, Node> > : virtual public Ifpack2::Preconditioner<HYPRE_Real,
-                                                                                                                    LocalOrdinal,
-                                                                                                                    HYPRE_BigInt,
-                                                                                                                    Node>,
-                                                                             virtual public Ifpack2::Details::CanChangeMatrix<Tpetra::RowMatrix<HYPRE_Real,
-                                                                                                                                                LocalOrdinal,
-                                                                                                                                                HYPRE_BigInt,
-                                                                                                                                                Node> > {
+                                                                                                                       LocalOrdinal,
+                                                                                                                       HYPRE_BigInt,
+                                                                                                                       Node>,
+                                                                                virtual public Ifpack2::Details::CanChangeMatrix<Tpetra::RowMatrix<HYPRE_Real,
+                                                                                                                                                   LocalOrdinal,
+                                                                                                                                                   HYPRE_BigInt,
+                                                                                                                                                   Node> > {
  public:
   //! \name Typedefs
   //@{
diff --git a/packages/ifpack2/src/Ifpack2_Hypre_def.hpp b/packages/ifpack2/src/Ifpack2_Hypre_def.hpp
index 048d02a5..7c8ed23d 100644
--- a/packages/ifpack2/src/Ifpack2_Hypre_def.hpp
+++ b/packages/ifpack2/src/Ifpack2_Hypre_def.hpp
@@ -395,7 +395,7 @@ int Hypre<Tpetra::RowMatrix<HYPRE_Real, LocalOrdinal, HYPRE_BigInt, Node> >::Set
     }
     HYPRE_BigInt GlobalRow[1];
     HYPRE_Int numEntries = (HYPRE_Int)indices.extent(0);
-    GlobalRow[0]  = GloballyContiguousRowMap_->getGlobalElement(i);
+    GlobalRow[0]         = GloballyContiguousRowMap_->getGlobalElement(i);
     IFPACK2_CHK_ERR(HYPRE_IJMatrixSetValues(HypreG_, 1, &numEntries, GlobalRow, new_indices.data(), values.data()));
   }
   IFPACK2_CHK_ERR(HYPRE_IJMatrixAssemble(HypreG_));
@@ -548,10 +548,10 @@ int Hypre<Tpetra::RowMatrix<HYPRE_Real, LocalOrdinal, HYPRE_BigInt, Node> >::Cal
 //==============================================================================
 template <class LocalOrdinal, class Node>
 void Hypre<Tpetra::RowMatrix<HYPRE_Real, LocalOrdinal, HYPRE_BigInt, Node> >::apply(const Tpetra::MultiVector<scalar_type, local_ordinal_type, global_ordinal_type, node_type> &X,
-                                                                                 Tpetra::MultiVector<scalar_type, local_ordinal_type, global_ordinal_type, node_type> &Y,
-                                                                                 Teuchos::ETransp mode,
-                                                                                 scalar_type alpha,
-                                                                                 scalar_type beta) const {
+                                                                                    Tpetra::MultiVector<scalar_type, local_ordinal_type, global_ordinal_type, node_type> &Y,
+                                                                                    Teuchos::ETransp mode,
+                                                                                    scalar_type alpha,
+                                                                                    scalar_type beta) const {
   using LO = local_ordinal_type;
   using SC = scalar_type;
   const std::string timerName("Ifpack2::Hypre::apply");
@@ -621,8 +621,8 @@ void Hypre<Tpetra::RowMatrix<HYPRE_Real, LocalOrdinal, HYPRE_BigInt, Node> >::ap
 //==============================================================================
 template <class LocalOrdinal, class Node>
 void Hypre<Tpetra::RowMatrix<HYPRE_Real, LocalOrdinal, HYPRE_BigInt, Node> >::applyMat(const Tpetra::MultiVector<scalar_type, local_ordinal_type, global_ordinal_type, node_type> &X,
-                                                                                    Tpetra::MultiVector<scalar_type, local_ordinal_type, global_ordinal_type, node_type> &Y,
-                                                                                    Teuchos::ETransp mode) const {
+                                                                                       Tpetra::MultiVector<scalar_type, local_ordinal_type, global_ordinal_type, node_type> &Y,
+                                                                                       Teuchos::ETransp mode) const {
   A_->apply(X, Y, mode);
 }  // applyMat()
 

More details about our use of clang-format and other tools can be found in the wiki.

[berselius]

I think this should work if you build trilinos with int int instead of int long long and configure hypre the same way...I can test it, but what is the recommended way to set that in spack for trilinos?

[cgcgcg]

I always built with --enable-bigint. #15085 (comment)

[berselius]

ahh, IIRC that one makes HYPRE_Int (locals) and HYPRE_BigInt (globals) both long long so I guess you also would not run into mismatches there. From what I've seen in the hypre docs going "full bigint" is not recommended/possible for most of their GPU-related things. And we don't need 64 bit locals in our app anyway.

[cgcgcg]

Ok. Sounds like your changes are an improvement, but might require additional changes to the Trilinos Spack package?

[berselius]

I guess it depends on how prescriptive you want to be about expected build failures, heh. In the current head I don't think there is a check that the hypre types and trilinos types are consistent.

[berselius]

Howdy @cgcgcg

I finally had time to take another look at the MR, and I’m not sure if I am remembering our discussion correctly. My sketched notes were that you suggested adding a typedef so that a developer can more easily toggle back to the way things were set up in the current head. But looking again I’m not sure that’s right. Apologies for thinking out loud below, let me lay out my thinking…

• Hypre does not have dynamic typing, HYPRE_Int and HYPRE_BigInt are locked in when you build it
• HYPRE_Int is the equivalent of Trilinos’s local_ordinal_type and HYPRE_BigInt is the equivalent of the global type in trilinos
• The issue this MR is fixing is that these mappings were not reflected in the interface above, but still work in the case where HYPRE_Int and HYPRE_BigInt are the same type (in your testing, IIRC they were both int64).
• Since this automatically picks up whatever was defined in the hypre build, I don't think it makes sense to add some additional typedef'ing

Let me know if I'm misremembering what we discussed, or if there was something else I should modify.

[cgcgcg]

Finally had a look:
https://github.com/hypre-space/hypre/blob/85b779557005b2eb94c231c1b516e988b87f4e53/src/utilities/HYPRE_utilities.h#L54-L95

Does this mean that we should outright specialize like so

template <class Node>
Hypre<Tpetra::RowMatrix<HYPRE_Real, HYPRE_Int, HYPRE_BigInt, Node> >

instead of

template <class LocalOrdinal, class Node>
Hypre<Tpetra::RowMatrix<HYPRE_Real, LocalOrdinal, HYPRE_BigInt, Node> >

?

[berselius]

@cgcgcg my app's tests are passing with these changes

[berselius]

Made some small changes as hypre@3.0: changes a header file name that ifpack2 references.

[berselius]

I've been testing these changes off of Trilinos 17.1.1

[cgcgcg]

@berselius LGTM. Please sign off your commits.

[berselius]

Might be my own ignorance here with the signoffs - I think the commits its complaining about was something I brought in with a merge or rebase. Is there an easy way to fix it?

[cgcgcg]

Oh, my bad. Yes, please rebase onto develop. That should get rid of the unsigned merge commits.