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.

Sign up for GitHub

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

Issue 4283: refactor muelu to not use dynamic profile when declaring crsgraph #4411

Merged
merged 3 commits into from
Apr 12, 2019
Merged

Issue 4283: refactor muelu to not use dynamic profile when declaring crsgraph #4411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6feef44
refactor muelu to not use dynamic profile when declaring crsgraph
GeoffDanielson Feb 15, 2019
91196bc
MueLu: correct num entries per row in static graph
jhux2 Mar 15, 2019
3c804b5
MueLu: correct num entries per row in static graph
jhux2 Mar 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/muelu/src/Graph/MueLu_CoalesceDropFactory_def.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -828,7 +828,7 @@ namespace MueLu {
GetOStream(Statistics1) << "CoalesceDropFactory: nodeMap " << nodeMap->getNodeNumElements() << "/" << nodeMap->getGlobalNumElements() << " elements" << std::endl;

// 3) create graph of amalgamated matrix
RCP<CrsGraph> crsGraph = CrsGraphFactory::Build(nodeMap, 10, Xpetra::DynamicProfile);
RCP<CrsGraph> crsGraph = CrsGraphFactory::Build(nodeMap, A->getNodeMaxNumRowEntries()*blockdim, Xpetra::StaticProfile);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code might run fine, but it could blow out memory if one row happens to have many more entries than other rows. Once we make this change, we may also want to duplicate the loop below, once to get a number of entries in each row (it looks like the loop is amenable to that), and once again to fill the CrsGraph.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhoemmen That's a good suggestion, I'll push an update.


LO numRows = A->getRowMap()->getNodeNumElements();
LO numNodes = nodeMap->getNodeNumElements();
Expand Down
2 changes: 1 addition & 1 deletion packages/muelu/src/Graph/StructuredAggregation/MueLu_AggregationStructuredAlgorithm_def.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ namespace MueLu {
myGraph = CrsGraphFactory::Build(rowMap,
colMap,
nnzOnRow,
Xpetra::DynamicProfile);
Xpetra::StaticProfile);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucbv should really comment on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My feeling is: this should be fine, the graph constructor actually allocates the exact amount of memory it needs since nnzOnRow is specified.
Of course I would still want this example to be tested to double check correctness.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lucbv :-)


*out << "Fill CrsGraph" << std::endl;
LO rowIdx = 0;
Expand Down
2 changes: 1 addition & 1 deletion packages/muelu/src/Misc/MueLu_ThresholdAFilterFactory_def.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ namespace MueLu {
// create new empty Matrix
RCP<const Map> rowmap = Ain->getRowMap();
RCP<const Map> colmap = Ain->getColMap();
RCP<CrsOMatrix> Aout = rcp(new CrsOMatrix(rowmap, expectedNNZperRow_ <= 0 ? Ain->getGlobalMaxNumRowEntries() : expectedNNZperRow_ , Xpetra::DynamicProfile));
RCP<CrsOMatrix> Aout = rcp(new CrsOMatrix(rowmap, expectedNNZperRow_ <= 0 ? Ain->getGlobalMaxNumRowEntries() : expectedNNZperRow_ , Xpetra::StaticProfile));
// loop over local rows
for(size_t row=0; row<Ain->getNodeNumRows(); row++)
{
Expand Down
2 changes: 1 addition & 1 deletion packages/muelu/src/Rebalancing/MueLu_IsorropiaInterface_def.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ namespace MueLu {


// 3) create graph of amalgamated matrix
RCP<CrsGraph> crsGraph = CrsGraphFactory::Build(nodeMap, 10, Xpetra::DynamicProfile);
RCP<CrsGraph> crsGraph = CrsGraphFactory::Build(nodeMap, A->getNodeMaxNumRowEntries()*blockdim, Xpetra::StaticProfile);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See note above. Can MueLu actually call Isorropia with a Tpetra matrix?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, MueLu will throw. See

Trilinos/packages/muelu/src/Rebalancing/MueLu_IsorropiaInterface_def.hpp

Lines 216 to 223 in 5c9a2ec

#ifdef HAVE_MUELU_TPETRA
#ifdef HAVE_MUELU_INST_DOUBLE_INT_INT
RCP< Xpetra::TpetraCrsGraph<LO, GO, Node> > tpCrsGraph = Teuchos::rcp_dynamic_cast<Xpetra::TpetraCrsGraph<LO, GO, Node> >(crsGraph);
TEUCHOS_TEST_FOR_EXCEPTION(tpCrsGraph != Teuchos::null, Exceptions::RuntimeError, "Tpetra is not supported with Isorropia.");
#else
TEUCHOS_TEST_FOR_EXCEPTION(false, Exceptions::RuntimeError, "Isorropia is an interface to Zoltan which only has support for LO=GO=int and SC=double.");
#endif // ENDIF HAVE_MUELU_INST_DOUBLE_INT_INT
#endif // ENDIF HAVE_MUELU_TPETRA

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhux2 Thanks for the clarification! It's probably better for even the Epetra-based code to use StaticProfile, as long as it has good bounds for each row.


// 4) do amalgamation. generate graph of amalgamated matrix
for(LO row=0; row<Teuchos::as<LO>(A->getRowMap()->getNodeNumElements()); row++) {
Expand Down