Tpetra::CrsGraph::globalAssemble broken with overlapping row Map (was: "Tpetra graph/matrix fill complete not communicating all data.")

[bathmatt]

I have a graph or a matrix which I use to get connectivity between elements and faces, I have rows which are faces and cols which are elements. I can fill this matrix and then when I do a fill complete it does not communicate the cols. The row graph is overlapping. The code is super short and pasted below. It is not clear from the documentation if this is supposed to work, but talking with people and reading the docs it looks like it should.

Here is the code with a matrix, but the graph code looks the same.

TEUCHOS_UNIT_TEST(tFullRelations, graph)
{
  typedef int LocalOrdinal;
  typedef int GlobalOrdinal;
  typedef Kokkos::Compat::KokkosDeviceWrapperNode<PHX::Device> NodeType;
  typedef Tpetra::Map<LocalOrdinal, GlobalOrdinal, NodeType> Map;
  typedef Tpetra::CrsGraph<LocalOrdinal, GlobalOrdinal, NodeType> Graph;
  typedef Tpetra::CrsMatrix<double, LocalOrdinal, GlobalOrdinal, NodeType, false> Matrix;
  typedef Tpetra::Export<LocalOrdinal, GlobalOrdinal, NodeType> Export;
  typedef Tpetra::Import<LocalOrdinal, GlobalOrdinal, NodeType> Import;

  int n=2*3;
  int rank, size;
  MPI_Comm_rank(MPI_COMM_WORLD, &rank);
  MPI_Comm_size(MPI_COMM_WORLD, &size);
  int dn = n/size;
  int start = dn*rank, end = dn*(rank+1)-1;

  std::vector<int> gids(dn), face_gids(dn+1);
  int face_offset = 2*n+123;
  for (int i=0;i<dn;++i) {
    gids[i] = start+i;
    face_gids[i] = gids[i]+face_offset;
  }
  face_gids[dn] = face_gids[dn-1]+1;

  Teuchos::RCP<const Teuchos::Comm<int>> comm(new Teuchos::MpiComm< int>(MPI_COMM_WORLD));

  auto elem_map = Teuchos::RCP<const Map>( new Map(Teuchos::OrdinalTraits<GlobalOrdinal>::invalid(), &gids[0], gids.size(), 0, comm ));
  auto face_map = Teuchos::RCP<const Map>( new Map(Teuchos::OrdinalTraits<GlobalOrdinal>::invalid(), &face_gids[0], face_gids.size(), 0, comm ));
  auto owned_face_map = Tpetra::createOneToOne(face_map);
  Matrix matrix(face_map, 2);

  for (int i=0;i<dn;++i){
    double one=1.0;
    int f1 = face_gids[i], f2 = face_gids[i+1];
    matrix.insertGlobalValues(f1, 1, &one, &gids[i]);
    matrix.insertGlobalValues(f2, 1, &one, &gids[i]);
    std::cout << "Rank "<<rank <<" Row "<< gids[i]<< " col " <<f1<< "\n";
    std::cout << "Rank "<<rank <<" Row "<< gids[i]<< " col " <<f2<< "\n";
  }  

  matrix.fillComplete(elem_map, owned_face_map);

  for (int i=0; i<face_gids.size(); ++i) {
    std::vector< GlobalOrdinal > indices(2);
    size_t num_ent;
    std::vector< double > vals(2);
    matrix.getGlobalRowCopy(face_gids[i], indices, vals, num_ent);
    assert(num_ent == 2 || num_ent == 1);

    std::cout << "Rank "<<rank <<" Row "<<face_gids[i] <<" col ";
    for (int i=0; i<num_ent; ++i)
      std::cout << indices[i] << " ";
    std::cout << std::endl;
  }
}

[mhoemmen]

@bathmatt Thanks for pointing this out. How urgent is this?

You tried both CrsGraph and CrsMatrix for this; neither worked correctly.
This could be an issue with globalAssemble.

[bathmatt]

I'm using the three graph solution, it isn't urgent-urgent as it is in
prototype code. But it will be checked in probably in a month and being
used.

On Thu, Sep 8, 2016 at 3:28 PM, Mark Hoemmen [email protected]
wrote:

@bathmatt https://github.com/bathmatt Thanks for pointing this out. How
urgent is this?

You tried both CrsGraph and CrsMatrix for this; neither worked correctly.
This could be an issue with globalAssemble.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#601 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AOPDICwyjl-kUpf_YvvxIieiAuF_kIIrks5qoH3rgaJpZM4J4So4
.

[mhoemmen]

The implementations of Crs{Graph,Matrix}::globalAssemble currently both assume that each process only needs to send information for a row to at most one process. This means that they likely won't work if the row Map is overlapping. I have changed the title to reflect this.

We've known this for a while. See e.g., Bugzilla Bug 5782, comment number 34 (and comments in the implementation of CrsMatrix::globalAssemble). The way I would fix this is first to Export to a graph/matrix with nonoverlapping row Map, then Import to a graph/matrix with the original, overlapping row Map. (One could skip the second Import if the original row Map is one to one.)

This is something you could do now, if you don't want to wait. We have not budgeted for this in our project plan, so we would need more information about urgency in order to increase this issue's priority.

I'm very much eager to get rid of both globalAssemble methods, for two reasons:

1. Users can get the same functionality and likely better performance by using Export (and possibly also Import), and
2. their implementations are awful (CrsGraph's implementation especially).

[bathmatt]

Is there a check for this to error out? It was very hard to debug and I
didn't see any docs on this in the user manual. So, at least a bug to
check is one-to-one should be there and abort in debug build.

So, if I make my first map oneToOne I can get rid of one graph and only do
one export.

On Thu, Sep 8, 2016 at 11:02 PM, Mark Hoemmen [email protected]
wrote:

The implementations of Crs{Graph,Matrix}::globalAssemble currently both
assume that each process only needs to send information for a row to at
most one process. This means that they likely won't work if the row Map is
overlapping. I have changed the title to reflect this.

We've known this for a while. See e.g., Bugzilla Bug 5782, comment number
34 (and comments in the implementation of CrsMatrix::globalAssemble). The
way I would fix this is first to Export to a graph/matrix with
nonoverlapping row Map, then Import to a graph/matrix with the original,
overlapping row Map. (One could skip the second Import if the original row
Map is one to one.)

This is something you could do now, if you don't want to wait. We have not
budgeted for this in our project plan, so we would need more information
about urgency in order to increase this issue's priority.

I'm very much eager to get rid of both globalAssemble methods, for two
reasons:

1. Users can get the same functionality and likely better performance
   by using Export (and possibly also Import), and
2. their implementations are awful (CrsGraph's implementation
   especially).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#601 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AOPDIMr_YVpbhZA7iB0GyGip8ZeIrHeeks5qoOhNgaJpZM4J4So4
.

[mhoemmen]

Whatever, I'm working on it right now.

[bathmatt]

Thanks,
I have the work around working with only one extra graph vs the initial two

On Fri, Sep 9, 2016 at 1:21 PM, Mark Hoemmen [email protected]
wrote:

Whatever, I'm working on it right now.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#601 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AOPDIJ1EMk_aC7VrSReaY65DgJGgAjaoks5qobG6gaJpZM4J4So4
.

[mhoemmen]

I split off the CrsMatrix::globalAssemble issue into #603.

I think I've fixed this one. I'll try the existing unit tests first, then I'll add some more unit tests in the case of an overlapping row Map (the current unit tests don't exercise this case, as far as I can tell -- that's probably why they passed before ;-) ).

[mhoemmen]

The fix passes existing unit tests.

[mhoemmen]

My fix had a minor bug in the case of an overlapping row Map. I added a unit test for that case, which caught the bug. I fixed the bug, and am running check-in tests now.

[mhoemmen]

I have the work around working with only one extra graph vs the initial two

Your work-around may still perform better, especially if you can avoid or amortize the createOneToOne call. Thus, I think it's actually the better option and you should leave it in the code if it at all makes sense.

[mhoemmen]

I just pushed a fix, with a unit test that passes.