Print global number of cells and dofs #1865

benegee · 2024-03-07T13:45:19Z

Resolves #1616

github-actions · 2024-03-07T13:45:33Z

Review checklist

This 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

The PR has a single goal that is clear from the PR title and/or description.
All code changes represent a single set of modifications that logically belong together.
No more than 500 lines of code are changed or there is no obvious way to split the PR into multiple PRs.

Code quality

The code can be understood easily.
Newly introduced names for variables etc. are self-descriptive and consistent with existing naming conventions.
There are no redundancies that can be removed by simple modularization/refactoring.
There are no leftover debug statements or commented code sections.
The code adheres to our conventions and style guide, and to the Julia guidelines.

Documentation

New functions and types are documented with a docstring or top-level comment.
Relevant publications are referenced in docstrings (see example for formatting).
Inline comments are used to document longer or unusual code sections.
Comments describe intent ("why?") and not just functionality ("what?").
If the PR introduces a significant change or new feature, it is documented in NEWS.md.

Testing

The PR passes all tests.
New or modified lines of code are covered by tests.
New or modified tests run in less then 10 seconds.

Performance

There are no type instabilities or memory allocations in performance-critical parts.
If the PR intent is to improve performance, before/after time measurements are posted in the PR.

Verification

The correctness of the code was verified using appropriate tests.
If new equations/methods are added, a convergence test has been run and the results
are posted in the PR.

Created with ❤️ by the Trixi.jl community.

codecov · 2024-03-07T14:12:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.12%. Comparing base (909abb4) to head (a158728).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1865      +/-   ##
==========================================
- Coverage   96.30%   96.12%   -0.19%     
==========================================
  Files         440      440              
  Lines       35793    35800       +7     
==========================================
- Hits        34470    34410      -60     
- Misses       1323     1390      +67

Flag	Coverage Δ
unittests	`96.12% <100.00%> (-0.19%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ncells was used elsewhere and has to be the local number

ranocha

Thanks! In #1616, you mentioned that the analysis callback also prints only the local information. Does this PR fix this as well?

src/semidiscretization/semidiscretization.jl

ranocha · 2024-03-08T06:52:21Z

src/semidiscretization/semidiscretization.jl

+"""
+    ndofsglobal(mesh, solver, cache)
+
+Return the global number of degrees of freedom associated with each scalar variable.
+Defaults to ndofs when there is no special implementation for parallel computations.
+"""
+@inline function ndofsglobal(mesh, solver, cache)


Suggested change

"""

ndofsglobal(mesh, solver, cache)

Return the global number of degrees of freedom associated with each scalar variable.

Defaults to ndofs when there is no special implementation for parallel computations.

"""

@inline function ndofsglobal(mesh, solver, cache)

"""

ndofsglobal(mesh, solver, cache)

Return the global number of degrees of freedom associated with each scalar variable.

Defaults to [`ndofs`](@ref) when there is no special implementation for

MPI-parallel computations.

"""

@inline function ndofsglobal(mesh, solver, cache)

But I'm not sure whether we should better turn this into a comment to avoid making this part of our official API. From my point of view, the methods accepting a semidiscretization are fine but this one is more like an implementation detail.

I am not sure about it.

My concern was that with ndofsglobal(semi::AbstractSemidiscretization) introduced above ndofsglobal(mesh, solver, cache) might get called with some mesh, solver, cache combinations for which it does not exist.

If I see it correctly, there is only
@inline function ndofsglobal(mesh, dg::DG, cache) in dg.jl.

What's you opinion, @sloede?

I'd consider it an implementation detail as well.

However, does it really need a fallback? Or, alternatively, could we check for mpi_isparallel() == 0 and otherwise error, such that a user cannot accidentally use that with a non-parallelized mesh?

I'd consider it an implementation detail as well.

Ok! However I am afraid I do not understand what this implies.

However, does it really need a fallback?

I added ndofsglobal(semi) to semidiscretization_hyperbolic.jl, which calls ndofsglobal(mesh, solver, cache). So when solver is not DG there should be a MethodError, shouldn't there?

Ah, the docstring! Thanks!

The problem is that I added ndofsglobal to
function Base.show
with the idea that always the global number of dofs gets printed, with ndofs as fallback in case there in no ndofsglobal.

An alternative could be to introduce a separate function just for printing, say print_ndofs, which would then check for mpi_isparallel() or have a default fallback.

Is this resolved after b883b3a?

It resolves the API thing:

Our current policy was that if it has a docstring, we consider it part of our API and then changing its API or its behavior would be a breaking change. Thus we don't add docstring to purely internal functions.

Otherwise this PR got stuck on the question of how to print the global number of dofs on Trixi's startup screen for a mesh which does not have a parallel implementation and thus nothing like ndofsglobal .

I'm a little confused and can't wrap my head around all the implications. Instead of writing back and forth, can we maybe discuss this at the next meeting (if that's not too late for you @benegee)?

Would it be a safe way out of this if we were to implement a fallback that checks mpi_isparallel() and errors if it is true? That is, if someone implements a mesh that is running in parallel but does not implement this function gets it thrown in the face?

Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>

JoshuaLampert · 2024-03-23T08:47:28Z

What about other mesh types like the TreeMesh? Does it print the local or global number of cells (see here)?

ranocha · 2024-03-25T16:52:43Z

The TreeMesh replicates all cell info on all ranks. Thus, it prints the global info.

src/semidiscretization/semidiscretization.jl

sloede · 2024-05-10T06:44:41Z

@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

function print_amr_information(callbacks, mesh, solver, cache)

Trixi.jl/src/callbacks_step/analysis.jl

Line 520 in 8a9fc7b

function print_amr_information(callbacks, mesh::P4estMesh, solver, cache)

Trixi.jl/src/callbacks_step/analysis.jl

Line 554 in 8a9fc7b

function print_amr_information(callbacks, mesh::T8codeMesh, solver, cache)

Otherwise we get a global element count but only rank-0 information on AMR, which is bound to cause confusion IMHO

benegee · 2024-05-10T06:52:05Z

True! I realized this in the meantime as well, but have not finished the MPI syncing of element counts.

sloede · 2024-05-10T06:53:40Z

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 MPI_Reduce call.

sloede · 2024-05-14T17:53:44Z

src/meshes/p4est_mesh.jl

@@ -105,7 +106,7 @@ function Base.show(io::IO, ::MIME"text/plain", mesh::P4estMesh)
    else
        setup = [
            "#trees" => ntrees(mesh),


Is the number of trees a local or a global information? If local --> please change to global

sloede · 2024-05-14T17:53:52Z

src/meshes/t8code_mesh.jl

@@ -91,7 +92,7 @@ function Base.show(io::IO, ::MIME"text/plain", mesh::T8codeMesh)
    else
        setup = [
            "#trees" => ntrees(mesh),


sloede · 2024-05-15T07:05:06Z

src/callbacks_step/analysis.jl

@@ -310,7 +310,7 @@ function (analysis_callback::AnalysisCallback)(u_ode, du_ode, integrator, semi)
        mpi_println("                 " * "              " *
                    "               " *
                    " PID:            " * @sprintf("%10.8e s", performance_index))
-        mpi_println(" #DOFs per field:" * @sprintf("% 14d", ndofs(semi)) *
+        mpi_println(" #DOFs per field:" * @sprintf("% 14d", ndofsglobal(semi)) *


Please check (if not yet done) that all other values that are printed by the analysis callback are also for the global problem and not rank local, otherwise people (especially me 😅) will get confused

sloede · 2024-05-15T07:05:29Z

src/semidiscretization/semidiscretization.jl

+"""
+    ndofsglobal(semi::AbstractSemidiscretization)
+
+Return the global number of degrees of freedom associated with each scalar variable.


Suggested change

Return the global number of degrees of freedom associated with each scalar variable.

Return the global number of degrees of freedom associated with each scalar variable across all MPI ranks.

Just to clarify what is meant by "global"

sloede · 2024-05-15T07:07:52Z

src/semidiscretization/semidiscretization_coupled.jl

+@inline function ndofsglobal(semi::SemidiscretizationCoupled)
+    sum(ndofsglobal, semi.semis)
+end


Suggested change

@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

sloede · 2024-05-15T07:08:33Z

src/semidiscretization/semidiscretization_hyperbolic.jl

@@ -314,7 +314,7 @@ function Base.show(io::IO, ::MIME"text/plain", semi::SemidiscretizationHyperboli

        summary_line(io, "source terms", semi.source_terms)
        summary_line(io, "solver", semi.solver |> typeof |> nameof)
-        summary_line(io, "total #DOFs per field", ndofs(semi))
+        summary_line(io, "total #DOFs per field", ndofsglobal(semi))


Please also make sure here that there are no rank-local quantities printed here

sloede · 2024-05-15T07:14:38Z

src/semidiscretization/semidiscretization.jl

+"""
+    ndofsglobal(mesh, solver, cache)
+
+Return the global number of degrees of freedom associated with each scalar variable.
+Defaults to ndofs when there is no special implementation for parallel computations.
+"""
+@inline function ndofsglobal(mesh, solver, cache)


Would it be a safe way out of this if we were to implement a fallback that checks mpi_isparallel() and errors if it is true? That is, if someone implements a mesh that is running in parallel but does not implement this function gets it thrown in the face?

src/semidiscretization/semidiscretization.jl

benegee added 3 commits March 7, 2024 14:40

switch to global count of cells

954a502

introduce ndofsglobal for generic types as fallback

ed6d5a9

switch to ndofsglobal for console output

0fd2376

benegee and others added 2 commits March 7, 2024 17:56

add ncellsglobal

51127eb

ncells was used elsewhere and has to be the local number

Merge branch 'main' into bg/print-global-number-of-cells-dofs

eef69b9

benegee marked this pull request as ready for review March 7, 2024 16:57

ranocha requested changes Mar 8, 2024

View reviewed changes

benegee and others added 3 commits March 8, 2024 12:24

Update src/semidiscretization/semidiscretization.jl

2efdfd8

Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>

remove docstring

b883b3a

ndofsglobal in analysis callback

6a48c77

DanielDoehring added the parallelization Related to MPI, threading, tasks etc. label Mar 12, 2024

ranocha reviewed Mar 25, 2024

View reviewed changes

src/semidiscretization/semidiscretization.jl Outdated Show resolved Hide resolved

benegee added 2 commits March 26, 2024 11:34

remove unnecessary fallback

d82aff5

Merge branch 'main' into bg/print-global-number-of-cells-dofs

a158728

ranocha requested a review from sloede March 26, 2024 11:24

sloede reviewed May 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print global number of cells and dofs #1865

Print global number of cells and dofs #1865

benegee commented Mar 7, 2024

github-actions bot commented Mar 7, 2024

codecov bot commented Mar 7, 2024 •

edited

ranocha left a comment

ranocha Mar 8, 2024 •

edited

benegee Mar 8, 2024 •

edited

ranocha Mar 8, 2024

sloede Mar 8, 2024

benegee Mar 8, 2024

benegee Mar 8, 2024

JoshuaLampert Mar 23, 2024

benegee Mar 25, 2024

sloede May 10, 2024

sloede May 15, 2024

JoshuaLampert commented Mar 23, 2024

ranocha commented Mar 25, 2024

sloede commented May 10, 2024

benegee commented May 10, 2024

sloede commented May 10, 2024

sloede May 14, 2024

sloede May 14, 2024

sloede May 15, 2024

sloede May 15, 2024

sloede May 15, 2024

sloede May 15, 2024

sloede May 15, 2024

	Return the global number of degrees of freedom associated with each scalar variable.
	Return the global number of degrees of freedom associated with each scalar variable across all MPI ranks.

Print global number of cells and dofs #1865

Are you sure you want to change the base?

Print global number of cells and dofs #1865

Conversation

benegee commented Mar 7, 2024

github-actions bot commented Mar 7, 2024

Review checklist

Purpose and scope

Code quality

Documentation

Testing

Performance

Verification

codecov bot commented Mar 7, 2024 • edited

Codecov Report

ranocha left a comment

Choose a reason for hiding this comment

ranocha Mar 8, 2024 • edited

Choose a reason for hiding this comment

benegee Mar 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaLampert commented Mar 23, 2024

ranocha commented Mar 25, 2024

sloede commented May 10, 2024

benegee commented May 10, 2024

sloede commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 7, 2024 •

edited

ranocha Mar 8, 2024 •

edited

benegee Mar 8, 2024 •

edited