-
Notifications
You must be signed in to change notification settings - Fork 112
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
Print global number of cells and dofs #1865
Conversation
Review checklistThis checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging. Purpose and scope
Code quality
Documentation
Testing
Performance
Verification
Created with ❤️ by the Trixi.jl community. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1865 +/- ##
==========================================
- Coverage 96.30% 96.11% -0.19%
==========================================
Files 440 460 +20
Lines 35793 36913 +1120
==========================================
+ Hits 34470 35478 +1008
- Misses 1323 1435 +112
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ncells was used elsewhere and has to be the local number
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.
Thanks! In #1616, you mentioned that the analysis callback also prints only the local information. Does this PR fix this as well?
Co-authored-by: Hendrik Ranocha <[email protected]>
What about other mesh types like the |
The |
@benegee Please note that you should also adapt the output of the AMR output, probably in these three functions: Trixi.jl/src/callbacks_step/analysis.jl Line 494 in 8a9fc7b
Trixi.jl/src/callbacks_step/analysis.jl Line 520 in 8a9fc7b
Trixi.jl/src/callbacks_step/analysis.jl Line 554 in 8a9fc7b
Otherwise we get a global element count but only rank-0 information on AMR, which is bound to cause confusion IMHO |
True! I realized this in the meantime as well, but have not finished the MPI syncing of element counts. |
Optimally, you'll use an implementation that only requires a single additional |
@inline function ndofsglobal(semi::SemidiscretizationCoupled) | ||
sum(ndofsglobal, semi.semis) | ||
end |
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.
@inline function ndofsglobal(semi::SemidiscretizationCoupled) | |
sum(ndofsglobal, semi.semis) | |
end | |
""" | |
ndofsglobal(semi::SemidiscretizationCoupled) | |
Return the global number of degrees of freedom associated with each scalar variable across all MPI ranks, and summed up over all coupled systems. | |
This is the same as [`ndofs`](@ref) for simulations running in serial or | |
parallelized via threads. It will in general be different for simulations | |
running in parallel with MPI. | |
""" | |
@inline function ndofsglobal(semi::SemidiscretizationCoupled) | |
sum(ndofsglobal, semi.semis) | |
end |
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 would actually argue that this docstring doesn't really help in practice since it's the same as the one for the AbstractSemidiscretization
- and SemidiscretizationCoupled <: AbstractSemidiscretization
. But I don't have a strong opinion on this. Shall we keep it, @sloede?
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.
Well, it explicitly mentions that it is the number of DOFs over all coupled systems. I don't have a strong opinion either, so I'll leave it up to @benegee to decide 🙂
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
…xi-framework/Trixi.jl into bg/print-global-number-of-cells-dofs
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
…xi-framework/Trixi.jl into bg/print-global-number-of-cells-dofs
SummaryGlobal number of elements and dofs
For Lines 461 to 463 in 76719a8
and Lines 520 to 522 in 76719a8
For Trixi.jl/src/callbacks_step/analysis_dgmulti.jl Lines 188 to 194 in 76719a8
I now added New solver types would now have to implement Global number of elements per levelThe local element numbers per level are now summed up across all ranks before being printed on rank 0. |
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.
Thanks a lot! Looks quite good to me 👍
@inline function ndofsglobal(semi::SemidiscretizationCoupled) | ||
sum(ndofsglobal, semi.semis) | ||
end |
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 would actually argue that this docstring doesn't really help in practice since it's the same as the one for the AbstractSemidiscretization
- and SemidiscretizationCoupled <: AbstractSemidiscretization
. But I don't have a strong opinion on this. Shall we keep it, @sloede?
Co-authored-by: Hendrik Ranocha <[email protected]>
Thanks a lot for tackling this @benegee! This makes Trixi.jl much more usable in parallel 💪 |
nelementsglobal was changed in v.0.7.16 trixi-framework/Trixi.jl#1865
Resolves #1616