Tpetra: replacing existing matrix entries via doImport fails

[jhux2]

Bug Report

@trilinos/tpetra

Description

I'm modifying the Ifpack2 Additive Schwarz class (part of #9606) to allow modification of the underlying matrix, as long as only existing matrix values are changed (i.e., the matrix graph must not change):

    A_ = Teuchos::rcp_dynamic_cast<const crs_matrix_type>(A, true);
    ExtMatrix_->resumeFill();
    ExtMatrix_->doImport(*A_, *ExtImporter_, Tpetra::REPLACE);
    ExtMatrix_->fillComplete(A_->getDomainMap(), RowMap_);

According to comments and code in TpetraCrsMatrix[def|decl].hpp, this is should be permissible as long as the graph of ExtMatrix_ is static. My understanding is that a CrsMatrix graph should always static after fillComplete, yet the code throws:

 p=0: *** Caught standard std::exception of type 'std::logic_error' :

  /ascldap/users/jhu/trilinos/Trilinos-dev/packages/tpetra/core/src/Tpetra_CrsMatrix_def.hpp:7330:

  Throw number = 1

  Throw test that evaluated to true: (! isStaticGraph () && combineMode == REPLACE)

  Tpetra::CrsMatrix<double, int, long long, Kokkos::Compat::KokkosDeviceWrapperNode<Kokkos::Serial, Kokkos::HostSpace> >::combineGlobalValues: REPLACE combine mode when the matrix has a dynamic graph is not yet implemented.

After talking it over with @csiefer2, it seems that the logic here https://github.com/trilinos/Trilinos/blob/e0adf23dfd61bca77cb13bc4c13f5432e188fd52/packages/tpetra/core/src/Tpetra_CrsMatrix_def.hpp#L7268 was never updated with the removal of dynamic graphs.

The comments in Tpetra_CrsMatrix_decl.hpp, e.g., https://github.com/trilinos/Trilinos/blob/e0adf23dfd61bca77cb13bc4c13f5432e188fd52/packages/tpetra/core/src/Tpetra_CrsMatrix_decl.hpp#L4046 are also seem to be out-of-date.

Blocks #9606.

[jhux2]

As an experiment, I changed the if test at https://github.com/trilinos/Trilinos/blob/e0adf23dfd61bca77cb13bc4c13f5432e188fd52/packages/tpetra/core/src/Tpetra_CrsMatrix_def.hpp#L7268 to always be true.

This allows modification of the matrix entries as described above (yeah!), but not unexpectedly, some Tpetra unit tests now fail:

The following tests FAILED:
     57 - TpetraCore_CrsMatrix_UnitTests2_MPI_4 (Failed)
     61 - TpetraCore_CrsMatrix_NonlocalAfterResume_MPI_4 (Failed)
     88 - TpetraCore_CrsMatrix_UnpackMerge_MPI_2 (Failed)
     92 - TpetraCore_Albany182_MPI_4 (Failed)
    121 - TpetraCore_ImportExport2_UnitTests_MPI_4 (Failed)
    177 - TpetraCore_MatrixMatrix_UnitTests_MPI_4 (Failed)

[jhux2]

This issue is still valid.

[brian-kelley]

@csiefer2 Bump; this issue is still affecting OverlappingRowMatrix in Ifpack2.

To summarize: doImport(REPLACE) doesn't change matrix structure, so why isn't it allowed when the CrsMatrix doesn't own its graph? isStaticGraph_() is required now.

Also, this is related to Karen's old issue #9655 : anything that just changes values shouldn't require resumeFill and fillComplete. Epetra doesn't require them, and as Karen found the fillComplete carries significant overhead.

[github-actions[bot]]

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity. If you would like to keep this issue open please add a comment and/or remove the MARKED_FOR_CLOSURE label. If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE. If it is ok for this issue to be closed, feel free to go ahead and close it. Please do not add any comments or change any labels or otherwise touch this issue unless your intention is to reset the inactivity counter for an additional year.

[github-actions[bot]]

This issue was closed due to inactivity for 395 days.

[vbrunini]

We're starting to look at a new performance test on the GPU with overlap enabled in the preconditioner and this is a bit of a hotspot. Can we reopen this @brian-kelley @csiefer2