search

trilinos / Trilinos

Primary repository for the Trilinos Project
https://trilinos.org/
Other
1.22k stars 568 forks source link

MueLu: Pcoarsen factory uses dynamic CrsGraph allocation #5625

Open lucbv opened 5 years ago

lucbv commented 5 years ago

Bug Report

@trilinos/muelu

Description

As can be seen here and here Pcoarsen factory does not pre-allocate memory before filling the prolongator matrix. This behavior is now forbidden and a proper number of non-zeros per row needs to be determined. This should be easy enough to fix by determining the max number of elements that share a node.

This work is part of #5614 to have MueLu tests pass after deprecated code in Tpetra is removed.

Steps to Reproduce

  1. SHA1: [insert here]
  2. Configure script: [attach here]
  3. Configure log: [attach here]
  4. Build log: [attach here]
  5. Input deck: [attach here]
  6. Do this.
  7. Do that.
  8. Shake fist angrily at computer.
  9. Run log: [attach here]
mhoemmen commented 5 years ago

This should be easy enough to fix by determining the max number of elements that share a node.

Be careful with increased memory use. I worry that this DynamicProfile deprecation will make MueLu's memory footprint blow up a lot.

lucbv commented 5 years ago

@mhoemmen, that could happen depending on how we handle that problem. The truth is. for our main algorithms such as TentativeP, SA and now GeometricInterpolation, we have already done enough optimization and use the expertStaticFillComplete where appropriate. The cost will mainly be felt for newer more experimental algorithms and as long as they do not have serious HPC customers it's probably fine...

mhoemmen commented 5 years ago

Just beware the "surprise customer" :-)

csiefer2 commented 5 years ago

This stuff still uses DynamicProfile. This got missed in the de-dynamic-ing?

mhoemmen commented 5 years ago

@csiefer2 A lot of tests were made to pass by just increasing the bound on the number of entries per row. This might have hidden issues on real problems.

lucbv commented 5 years ago

@csiefer2 @mhoemmen I am a bit suspicious of this line of code as well as this one. They both point to the following constructor in Xpetra which uses a DynamicProfile by default. By the way it seems that most Xpetra matrix constructors are still using DynamicProfile by default and that profile type is likely propagated to Tpetra...

I am somewhat surprised that this does not generate more problems in MueLu?

github-actions[bot] commented 3 years ago

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[bot] commented 3 years ago

This issue was closed due to inactivity for 395 days.

jhux2 commented 3 years ago

Reopening until verified as fixed.