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: Deprecated DynamicProfile used without deprecation guards #5065

Closed
kddevin opened this issue May 1, 2019 · 7 comments
Closed

Tpetra: Deprecated DynamicProfile used without deprecation guards #5065

kddevin opened this issue May 1, 2019 · 7 comments
Assignees
Labels
pkg: Tpetra type: bug The primary issue is a bug in Trilinos code or tests

Comments

@kddevin
Copy link
Contributor

kddevin commented May 1, 2019

Bug Report

@trilinos/tpetra

Description

DynamicProfile is a deprecated feature.
It is used in Tpetra_CrsMatrix_decl.hpp without deprecation guards.

Thus, building with TPETRA_ENABLE_DEPRECATED_CODE=OFF fails.

Related to #4701

Steps to Reproduce

Build Tpetra with TPETRA_ENABLE_DEPRECATED_CODE=OFF.
I am building with many other packages; I don't know if they are needed to see the error.

-D Tpetra_ENABLE_DEPRECATED_CODE:BOOL=OFF \
-D Teuchos_HIDE_DEPRECATED_CODE:BOOL=ON \
\
-D Trilinos_ENABLE_Stokhos:BOOL=ON \
-D Trilinos_ENABLE_Nox:BOOL=ON \
-D Trilinos_ENABLE_ROL:BOOL=ON \
-D ROL_ENABLE_EXAMPLES:BOOL=OFF \
-D Trilinos_ENABLE_MiniTensor:BOOL=OFF \
-D ROL_ENABLE_MiniTensor:BOOL=OFF \
-D Trilinos_ENABLE_Panzer:BOOL=ON \
-D Trilinos_ENABLE_PanzerAdaptersSTK:BOOL=OFF \
-D Trilinos_ENABLE_PanzerAdaptersIOSS:BOOL=OFF \
-D Trilinos_ENABLE_Thyra:BOOL=ON \
-D Trilinos_ENABLE_MueLu:BOOL=ON \
-D Trilinos_ENABLE_Anasazi:BOOL=ON \
-D Trilinos_ENABLE_Belos:BOOL=ON \
-D Trilinos_ENABLE_TrilinosCouplings:BOOL=ON \
-D Trilinos_ENABLE_STKIO:BOOL=OFF \
-D Trilinos_ENABLE_STKUtil:BOOL=OFF \
@kddevin kddevin added type: bug The primary issue is a bug in Trilinos code or tests pkg: Tpetra labels May 1, 2019
@kddevin
Copy link
Contributor Author

kddevin commented May 1, 2019

This one may be difficult to fix, as it is used without max number of entries per row throughout the Trilinos stack. We may need help from other package developers to fix it correctly.

Not sure how this one got through testing with TPETRA_ENABLE_DEPRECATED_CODE=OFF, though, as we use this method in Tpetra tests.

@william76
Copy link
Contributor

FYI, I don't think any of the PR tests ever set TPETRA_ENABLE_DEPRECATED_CODE=OFF so folks will probably want to run a sanity check on this locally before submitting PR's

@kddevin
Copy link
Contributor Author

kddevin commented May 2, 2019

@mhoemmen @jhux2 @csiefer2 @GeoffDanielson
Let's discuss possible solutions here.

From the Tpetra side, I think the fix is straightforward:

  • In createCrs*, require the user to provide a maxNumEntriesPerRow (deprecating the default value of 0) and then use StaticProfile
  • In Tpetra tests, most tests provide the maxNumEntriesPerRow; for any that don't, we can figure out the appropriate value

For packages that use Tpetra::createCrs*, figuring out the appropriate value may be more difficult.
I see use of createCrsMatrix and createCrsGraph in

  • @trilinos/muelu (MueLu_MatLabUtils_def.hpp)
  • @trilinos/teko for MatrixMatrix multiplication, building block matrices, tests; some instances provide a size but many do not
  • @trilinos/stokhos (Stokhos_Tpetra_Utilities_UQ_PCE.hpp)

@jhux2
Copy link
Member

jhux2 commented May 2, 2019

For packages that use Tpetra::createCrs*, figuring out the appropriate value may be more difficult.
I see use of createCrsMatrix and createCrsGraph in

  • @trilinos/muelu (MueLu_MatLabUtils_def.hpp)

This looks simple to fix. I'll submit a PR.

@mhoemmen
Copy link
Contributor

mhoemmen commented May 2, 2019

@kddevin @jhux2 Teko should just use the sparse matrix-matrix interface that creates and returns a new CrsMatrix, not the "create an empty matrix and then fill it in" interface which is not idiomatic C++ anyway.

@kddevin
Copy link
Contributor Author

kddevin commented May 2, 2019

Thanks @mhoemmen

Can you point me to this interface? All I see is interface that requires a matrix for C.

@mhoemmen
Copy link
Contributor

mhoemmen commented May 3, 2019

@kddevin Huh, I didn't realize we didn't have one of those. @trilinos/muelu I thought we had an interface like that?

These interfaces might work if we just create an empty matrix, pass it in as the nonconst reference (output argument), and let MueLu do its setAllValues and expertStaticFillComplete thing. The idea is that MueLu expects an empty matrix and just clobbers it with raw arrays, so you don't need to preallocate storage.

kddevin added a commit that referenced this issue Jun 9, 2019
Tpetra DynamicProfile deprecation:  some fixes for #5065
jmgate pushed a commit to tcad-charon/Trilinos that referenced this issue Jun 27, 2019
…s:develop' (2a84506).

* trilinos-develop: (45 commits)
  Tpetra - I forgot to check stokhos.
  Tpetra - remove experimental block crs stuffs
  Ifpack2:  responding to @mhoemmen 's review in trilinos#5426
  Intrepid2: parallelize the Lagragian Interpolation tool (trilinos#5431)
  Tpetra - remove experimental block crs stuffss in other packages
  tacho: Replace deleted TaskSchedulerType alias
  tacho: remove unused scheduler_name from unit test file
  Tpetra - backward compatibility to experimental namespace
  Ifpack2:  bug fix for sparse container staticProfile construction
  tacho: Fix -Werror
  For you Paul ;)
  Tacho - do not include test and example if rdc is off.
  Disable Tacho+Cuda testing until RDC is enabled
  Snapshot of kokkos-kernels.git from commit d86db111124cea12e23dd3447b6c307f96ef7439
  Snapshot of kokkos.git from commit 2983b80d9aeafabb81f2c8c1c5a49b40cc0856cb
  ifpack2:  more work toward static profile trilinos#5426 A count-allocate-fill pattern for SparseContainer. These changes may not be well tested yet; I am working my way through the tests.
  ifpack2:  minor fixes for deprecating dynamic profile
  ifpack2:  removed extraneous matrix creation (good catch, @mhoemmen) trilinos#5426 Removed RCPNode error
  bddc: Update interface for Tacho::Solver
  ifpack2:  revisions to support static profile (round 1:  ILUT) trilinos#5065
  ...
jmgate pushed a commit to tcad-charon/Trilinos that referenced this issue Jun 27, 2019
…s:develop' (2a84506).

* trilinos-develop: (45 commits)
  Tpetra - I forgot to check stokhos.
  Tpetra - remove experimental block crs stuffs
  Ifpack2:  responding to @mhoemmen 's review in trilinos#5426
  Intrepid2: parallelize the Lagragian Interpolation tool (trilinos#5431)
  Tpetra - remove experimental block crs stuffss in other packages
  tacho: Replace deleted TaskSchedulerType alias
  tacho: remove unused scheduler_name from unit test file
  Tpetra - backward compatibility to experimental namespace
  Ifpack2:  bug fix for sparse container staticProfile construction
  tacho: Fix -Werror
  For you Paul ;)
  Tacho - do not include test and example if rdc is off.
  Disable Tacho+Cuda testing until RDC is enabled
  Snapshot of kokkos-kernels.git from commit d86db111124cea12e23dd3447b6c307f96ef7439
  Snapshot of kokkos.git from commit 2983b80d9aeafabb81f2c8c1c5a49b40cc0856cb
  ifpack2:  more work toward static profile trilinos#5426 A count-allocate-fill pattern for SparseContainer. These changes may not be well tested yet; I am working my way through the tests.
  ifpack2:  minor fixes for deprecating dynamic profile
  ifpack2:  removed extraneous matrix creation (good catch, @mhoemmen) trilinos#5426 Removed RCPNode error
  bddc: Update interface for Tacho::Solver
  ifpack2:  revisions to support static profile (round 1:  ILUT) trilinos#5065
  ...
@kddevin kddevin closed this as completed Jan 14, 2020
@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
Labels
pkg: Tpetra type: bug The primary issue is a bug in Trilinos code or tests
Projects
Status: Done
Development

No branches or pull requests

6 participants