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: CsrMatrix structure cannot be changed after first call to fillComplete() #12211

Open
kyrillh opened this issue Sep 6, 2023 · 7 comments
Assignees
Labels
impacting: documentation The issue is primarily about documentation vs. a problem with the build or tests MARKED_FOR_CLOSURE Issue or PR is marked for auto-closure by the GitHub Actions bot. pkg: Tpetra

Comments

@kyrillh
Copy link

kyrillh commented Sep 6, 2023

cc: @csiefer2 @searhein

The Tpetra::CrsMatrix documentation says here regarding resumeFill() that it "Resume operations that may change the values or structure of the matrix."

Accordingly, I assumed that e.g. insertLocalValues() could be used after calling resumeFill(), provided the matrix was not created with a const CrsGraph. However, this is not the case as I found out through multiple error messages. As mentioned in this issue, after the first call to fillComplete() the matrix structure cannot be changed anymore.

It would be great if the documentation clarified that structure-changing functions (e.g. insertLocalValues(), doImport() etc.) can only be used before first calling fillComplete() and that resumeFill() must be used to change matrix entries but not its structure.

@kyrillh kyrillh added the impacting: documentation The issue is primarily about documentation vs. a problem with the build or tests label Sep 6, 2023
@cwpearson cwpearson self-assigned this Sep 20, 2023
@cwpearson
Copy link
Contributor

cwpearson commented Sep 20, 2023

Things we're apparently allowed to do after resumeFill():

  • removeLocalIndices(): core/test/CrsGraph/CrsGraph_UnitTests0.cpp:247
  • setAllToScalar(): core/example/advanced/Benchmarks/CrsMatrixDenseRowUnpack.cpp:621
  • sumIntoLocalValues(): core/example/advanced/Benchmarks/CrsMatrix_sumIntoLocalValues.cpp:160
  • replaceGlobalValues(): core/example/Lesson03-Power-Method/lesson03_power_method.cpp:359
  • modify entries in the values view: core/src/Tpetra_applyDirichletBoundaryCondition.hpp:232
  • reindexColumns(): core/test/CrsGraph/CrsGraph_ReindexColumns.cpp:285
  • insertLocalIndices(): core/test/CrsGraph/CrsGraph_UnitTests1.cpp:215
  • setAllIndices(): core/test/CrsGraph/CrsGraph_UnitTests1.cpp:556
  • insertLocalValues(): core/test/CrsMatrix/CrsMatrix_MultipleFillCompletes.cpp:131
  • insertGlobalValues(): core/test/CrsMatrix/CrsMatrix_NonlocalAfterResume.cpp:203
  • transformLocalValues(): core/test/CrsMatrix/CrsMatrix_TransformValues.cpp:124
  • scale(), globalAssemble(), replaceLocalValues(): core/test/CrsMatrix/CrsMatrix_UnitTests4.cpp:411-416
  • sumIntoGlobalValues(): core/test/MatrixMatrix/FECrs_MatrixMatrix_UnitTests.cpp:229
  • Output of Tpetra::TripleMatrixMultiply::MultiplyRAP: core/test/MatrixMatrix/MatrixMatrix_UnitTests.cpp:616
  • Output of Tpetra::MatrixMatrix::Multiply: core/test/MatrixMatrix/MatrixMatrix_UnitTests.cpp:693

@cwpearson
Copy link
Contributor

cwpearson commented Sep 20, 2023

Files that may need clarifying updates to documentation and/or comments

  • core/example/Lesson03-Power-Method/index.doc
  • core/example/Lesson03-Power-Method/lesson03_power_method.cpp
  • core/example/Lesson04-Sparse-Matrix-Fill/index.doc
  • core/ext/TpetraExt_MatrixMatrix_decl.hpp
  • core/guide/src/Examples/SourceCode/power_method_1.cpp
  • core/guide/src/SparseMatrices/Creating/FillComplete.rst
  • core/src/Tpetra_BlockCrsMatrix_decl.hpp
  • core/src/Tpetra_CrsGraph_decl.hpp
  • core/src/Tpetra_CrsMatrix_decl.hpp
  • core/src/Tpetra_FECrsMatrix_decl.hpp
  • core/test/CrsMatrix/CrsMatrix_MultipleFillCompletes.cpp
  • core/test/CrsMatrix/CrsMatrix_NonlocalAfterResume.cpp

@cwpearson
Copy link
Contributor

cwpearson commented Sep 20, 2023

@kyrillh can you comment on how what you're doing is different than this example in the unit tests of insertLocalValues after resumeFill?

matrix.resumeFill (); // Now there is room for more entries
if (myRank == 0) {
cerr << " Test that insertLocalValues does not throw" << endl;
}
for (LO r = 0; r < static_cast<LO> (numLocal); ++r) {
TEST_NOTHROW( matrix.insertLocalValues (r, tuple (r), tuple (ST::one ())) );
}

@kyrillh
Copy link
Author

kyrillh commented Sep 21, 2023

I was calling the initial fillComplete() without passing any parameters. I just tried explicitly setting "Optimize Storage" to false and was able to call doExport()/doImport()/insertLocalValues() without error as in the test.

@cwpearson
Copy link
Contributor

Okay, great. We can still improve the documentation to help out with this in the future

@cwpearson
Copy link
Contributor

Possibly related: relationship between "static profile" and optimize storage in fillLocalGraph(). Documentation may not have caught up with the removal of something called "dynamic profile," where you were allowed to do operations that caused the graph to get reallocated

@jhux2 jhux2 added this to Tpetra Aug 12, 2024
@jhux2 jhux2 moved this to Backlog in Tpetra Aug 12, 2024
Copy link

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 github-actions bot added the MARKED_FOR_CLOSURE Issue or PR is marked for auto-closure by the GitHub Actions bot. label Sep 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impacting: documentation The issue is primarily about documentation vs. a problem with the build or tests MARKED_FOR_CLOSURE Issue or PR is marked for auto-closure by the GitHub Actions bot. pkg: Tpetra
Projects
Status: Backlog
Development

No branches or pull requests

3 participants