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
Matrix memory and n_nonzeros postprocessor. #1105
Conversation
/run-tests |
Nice! This will be really useful to debug the DG sparsity patterns. |
I am okay to merge as is when the tests pass. @bangerth what do you think about adding a test to make sure we won't mess up the matrix in the future? |
I should probably update this to only output 2.d.p. for memory usage, in the vein of #1078. I'll push that in here as soon as I'm happy it works. |
Updated to only output 2 d.p. |
Looks good to me. I'm usually not the biggest fan of using screen output space, but in this case it's just a debugging aid and not meant to be informative to the average user. So this works for me. I'd very much like to have a test for this. @spco -- can you add something like what you already show? The only test output worth saving is the |
The |
Hm, I would usually see whether @tjhei could take a look, but he's offline for the next couple of weeks. Let's ignore this issue for now. Is the patch ready otherwise? |
Yes it's ready, unless we want to add the test I mentioned in #1051 in here. |
Does @ian-r-rose have any idea what might be happening, to make this test not create those files? |
{ | ||
private: | ||
std::string | ||
get_stats(const TrilinosWrappers::BlockSparseMatrix &matrix, std::string matrix_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did apparently not look closely enough to this patch. In this function, can you:
- make the second argument a const reference
- break arguments onto separate lines
- I suspect that the function could be made
const
itself. In fact, I think the function doesn't actually have to be a member function, in which case it could just as well live in an anonymous namespace in the.cc
file, rather than being part of the public interface.
Let's keep #1051 separate. We can always add tests at a later time in independent pull requests. |
This latest should be good to go once all-1 tests have passed. I've pulled get_stats() out into an anonymous namespace, which just requires the MPI_COMM to be passed in, and fixed the constness/alignment. I agree on keeping #1051 separate. |
const std::string | ||
get_stats(const aspect::TrilinosWrappers::BlockSparseMatrix &matrix, | ||
const std::string matrix_name, | ||
const MPI_Comm &comm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't actually a need to forward declare this function. In fact, forward declaring things in anonymous namespaces has no useful effect because every other .cc
file that may #include
this one would see this function in a different namespace than where it really is (because every .cc
has a unique anonymous namespace). Just remove the declaration.
Real close now! |
Such a taskmaster! ;) Fixed, and squashed. And something learned about forward declarations! Thanks. |
Yes, sorry. I'm a stickler for details :-( It makes me a good programmer, but a poor collaborator :-( |
Addresses #1053. Outputs to screen. To help with what this looks like: