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

Tpetra: how to copy a matrix with a different range/domain map without invalidating the original #9391

Closed
bathmatt opened this issue Jul 6, 2021 · 18 comments

Comments

@bathmatt
Copy link
Contributor

bathmatt commented Jul 6, 2021

Question

@trilinos/tpetra
@jhux2 @cgcgcg @kddevin

So, I;m removing UVM from zoltan2 and I ran into a use case in Z2 that is problematic for UVM removal, They have two matrices

JBlock and JCyclic where the only difference is the rance and domain maps. They construct the JCyclic via the call below

   JCyclic = rcp(new matrix_t(JBlock->getLocalMatrixDevice(),
                               JBlock->getRowMap(), JBlock->getColMap(),
                              vMapCyclic, wMapCyclic));

This does a shallow copy of the device matrix and then sets the use counts for the graph and values to 8 for the device side and only 4 for the host side.

I tried the following, make a deep copy of the matrix since this is just in a test and who cares about memory


    {
      { auto fff = JBlock->getLocalMatrixHost();}
      matrix_t JBlockCopy(*JBlock, Teuchos::Copy);
      { auto fff = JBlock->getLocalMatrixHost();}
      JCyclic = rcp(new matrix_t(JBlockCopy.getLocalMatrixDevice(),
                                 JBlock->getRowMap(), JBlock->getColMap(),
                                 vMapCyclic, wMapCyclic));
    }
      { auto fff = JBlock->getLocalMatrixHost();}

and the 3rd getLocalMatrixHost fails cuz the graph has a mismatched use count. So, the graph is always a shallow copy it looks like

The auto fff = ... is there just for use count checking, just FYI

So, there has to be a way to do what I want, I looked there is no setRangeMap and only getters or I would just copy the matrix raising the counts to 8 but evenly.

Thanks

@cgcgcg
Copy link
Contributor

cgcgcg commented Jul 6, 2021

Not sure about this, but what happens if you use the CrsMatrix copy constructor with copyOrView=Teuchos::View and then do a fillResume and a fillComplete with the different domain and range maps?

@bathmatt
Copy link
Contributor Author

bathmatt commented Jul 6, 2021

Not sure about this, but what happens if you use the CrsMatrix copy constructor with copyOrView=Teuchos::View and then do a fillResume and a fillComplete with the different domain and range maps?

Look at you with all the answers :)
TEST PASSED

@bathmatt
Copy link
Contributor Author

bathmatt commented Jul 6, 2021

Not sure about this, but what happens if you use the CrsMatrix copy constructor with copyOrView=Teuchos::View and then do a fillResume and a fillComplete with the different domain and range maps?

Look at you with all the answers :)
TEST PASSED

Wait @cgcgcg you're not so smart as of yet, when I run in parallel I get failures in the fill complete call

Throw test that evaluated to true: (! domainMapsMatch)

Tpetra::CrsMatrix<double, int, long long, Kokkos::Compat::KokkosDeviceWrapperNode<Kokkos::Cuda, Kokkos::CudaSpace> >::fillComplete: The CrsMatrix's domain Map
 does not match the graph's domain Map.  The graph cannot be changed because it was given to the CrsMatrix constructor as const.  You can fix this by passing 
in the graph's domain Map and range Map to the matrix's fillComplete call.
terminate called after throwing an instance of 'std::runtime_error'
  what():  /scratch/mbetten/Trilinos/Trilinos/packages/tpetra/core/src/Tpetra_CrsMatrix_def.hpp:4881:

looking into why

@cgcgcg
Copy link
Contributor

cgcgcg commented Jul 6, 2021

Hm. There is also a replaceDomainMapAndImporter in case it's just the domain map that needs to be changed.

@bathmatt
Copy link
Contributor Author

bathmatt commented Jul 6, 2021

Here is the code I replaced it with

     // Make JCyclic:  same matrix with different Domain and Range maps
-    JCyclic = rcp(new matrix_t(JBlock->getLocalMatrixDevice(),
-                               JBlock->getRowMap(), JBlock->getColMap(),
-                               vMapCyclic, wMapCyclic));
+
+    JCyclic = rcp(new matrix_t(*JBlock,Teuchos::View));
+    JCyclic->resumeFill();
+    JCyclic->fillComplete(vMapCyclic, wMapCyclic);

@cgcgcg
Copy link
Contributor

cgcgcg commented Jul 6, 2021

Another attempt: Construct a new CrsGraph object using the local graph of the first matrix and different domain and range maps. Then construct a new CrsMatrix using the new graph and the values.

@bathmatt
Copy link
Contributor Author

bathmatt commented Jul 6, 2021

Another attempt: Construct a new CrsGraph object using the local graph of the first matrix and different domain and range maps. Then construct a new CrsMatrix using the new graph and the values.

Yeah, i thought about all that crap, i can't use a local graph but will look into some other way to rebuild the matrix.

Hoping for a good idea from someone hoping not to have to do all that crap

@kddevin
Copy link
Contributor

kddevin commented Jul 6, 2021

@trilinos/tpetra should look at this case. Either tpetra should correctly handle the use-case or it should deprecate that constructor.

We had to do some workaround for FECrsMatrix; let us take a look.

@bathmatt
Copy link
Contributor Author

bathmatt commented Jul 7, 2021

@JacobDomagala

@bathmatt
Copy link
Contributor Author

@trilinos/tpetra any comment on the comment by @kddevin ??? I can make a copy of the graph and fill in the values and do all the stuff to recreate the matrix. It is in a test so it isn't a big deal that we duplicate the entire matrix but if this has a real application we will need a better path forward

@bathmatt
Copy link
Contributor Author

@cgcgcg or anyone else, is there an easy way to copy one matrix values to another matrix with the same graph

Was hoping for a getValues and a deep copy of some sort.

If I was to use an importer, do I just create an importer with the row maps?

@rppawlo
Copy link
Contributor

rppawlo commented Jul 15, 2021

getLocalMatrixDevice() returns the KokkosSparse::CrsMatrix. The matrix values in this object are stored in a Kokkos::View. I don't see an accessor to get the entire view (there are row view accessors), so you may need to add a new method for that. Since the object is snapshotted into trilinos you would have to get the changes into kokkos kernels first.

@cgcgcg
Copy link
Contributor

cgcgcg commented Jul 15, 2021

I think the values view in the local matrix is accessible.

@rppawlo
Copy link
Contributor

rppawlo commented Jul 15, 2021

@cgcgcg is correct. The class members are public. Just grab the "values" member.

@bathmatt
Copy link
Contributor Author

This is what I've done to replace that simple single line

   // Make JCyclic:  same matrix with different Domain and Range maps                                                                                        
    RCP<const graph_t> block_graph = JBlock->getCrsGraph();
    RCP<graph_t> cyclic_graph = rcp(new graph_t(*block_graph));
    cyclic_graph->resumeFill();
    cyclic_graph->fillComplete(vMapCyclic, wMapCyclic);
    JCyclic = rcp(new matrix_t(cyclic_graph));
    JCyclic->resumeFill();
    TEUCHOS_ASSERT(block_graph->getNodeNumRows() == cyclic_graph->getNodeNumRows());
    {
      auto val_s = JBlock->getLocalMatrixHost().values;
      auto val_d = JCyclic->getLocalMatrixHost().values;
      TEUCHOS_ASSERT(val_s.extent(0) == val_d.extent(0));
      Kokkos::deep_copy(val_d, val_s);
    }
    JCyclic->fillComplete();
  }


@kddevin
Copy link
Contributor

kddevin commented Jul 19, 2021

We decided to add methods to replace the Domain and Range map. (Epetra has these functions, but Tpetra does not.) Then one would use the shallow copy constructor followed by the methods for replacing the Domain and Range map. See #9451

@bathmatt
Copy link
Contributor Author

bathmatt commented Jul 19, 2021 via email

@kddevin
Copy link
Contributor

kddevin commented Aug 30, 2021

In branch tpetra_9391, there is a test that attempts to copy a matrix four different ways. Two work -- repeating all the inserts to build the new matrix from scratch

TEUCHOS_UNIT_TEST_TEMPLATE_4_DECL(CrsMatrix, Bug9391_Scratch, SC, LO, GO, NT)
and @bathmatt 's method above
TEUCHOS_UNIT_TEST_TEMPLATE_4_DECL(CrsMatrix, Bug9391_CopyGraph, SC, LO, GO, NT)

Two do not work: building the new matrix from the original's local device matrix gets the counts wrong so that one cannot take a host view

TEUCHOS_UNIT_TEST_TEMPLATE_4_DECL(CrsMatrix, Bug9391_LocalMat, SC, LO, GO, NT)
and calling the copy constructor for a "deep" copy followed by replaceDomainMap, replaceRangeMap creates a static graph for which the maps cannot be changed
TEUCHOS_UNIT_TEST_TEMPLATE_4_DECL(CrsMatrix, Bug9391_Replace, SC, LO, GO, NT)
That is, our proposed solution above will not work, as the "deep" copy constructor actually deep-copies only the values of the matrix, not the graph.

@csiefer2 csiefer2 closed this as completed Jul 5, 2022
@jhux2 jhux2 added this to Tpetra Aug 12, 2024
@jhux2 jhux2 moved this to Done in Tpetra Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

6 participants