Fix noncons mortars for remaining mesh types #2134

andrewwinters5000 · 2024-10-29T10:23:45Z

This is a 2D analog of #2127 .

P4estMesh 2D
TreeMesh 2D
TreeMesh 3D

The downstream tests for Trixi2Vtk are failing because the MHD rotor output was used for testing of these conversion routines. So the reference files for Trixi2Vtk would need to be updated.

Looking toward fixing the TreeMesh, these should not mess with the Trixi2Vtk testing because the reference files used there are for Euler runs.

github-actions · 2024-10-29T10:24:03Z

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 with its PR number.

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-10-29T11:04:29Z

Codecov Report

Attention: Patch coverage is 92.22222% with 14 lines in your changes missing coverage. Please review.

Project coverage is 89.93%. Comparing base (c45f919) to head (b20e587).

Files with missing lines	Patch %	Lines
src/solvers/dgsem_tree/dg_2d_parallel.jl	0.00%	12 Missing ⚠️
src/solvers/dgsem_p4est/dg_2d_parallel.jl	0.00%	2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (c45f919) and HEAD (b20e587). Click for more details.

HEAD has 3 uploads less than BASE

Flag BASE (c45f919) HEAD (b20e587)

unittests 26 23

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2134      +/-   ##
==========================================
- Coverage   96.36%   89.93%   -6.43%     
==========================================
  Files         477      477              
  Lines       37760    37823      +63     
==========================================
- Hits        36385    34015    -2370     
- Misses       1375     3808    +2433

Flag	Coverage Δ
unittests	`89.93% <92.22%> (-6.43%)`	⬇️

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.

amrueda

Looks great to me!
I just have a couple of questions:

src/solvers/dgsem_p4est/dg_2d.jl

test/test_parabolic_2d.jl

amrueda

Looks good to me! Thanks for adding this, @andrewwinters5000!

All my comments are about the repeated flux computations. One possibility would be to merge this as is, and create an issue (and maybe a new PR, if you have time/interest) about that. Would that be a good way to proceed, @sloede @ranocha?

amrueda · 2024-11-08T15:01:00Z

src/solvers/dgsem_tree/dg_2d.jl

-        calc_fstar!(fstar_upper, equations, surface_flux, dg, u_upper, mortar,
+        calc_fstar!(fstar_primary_upper, equations, surface_flux, dg, u_upper, mortar,
                    orientation)
-        calc_fstar!(fstar_lower, equations, surface_flux, dg, u_lower, mortar,
+        calc_fstar!(fstar_primary_lower, equations, surface_flux, dg, u_lower, mortar,
+                    orientation)
+        calc_fstar!(fstar_secondary_upper, equations, surface_flux, dg, u_upper, mortar,
+                    orientation)
+        calc_fstar!(fstar_secondary_lower, equations, surface_flux, dg, u_lower, mortar,
                    orientation)


I suggest to modify the function calc_fstar!, such that it takes fstar_primary and fstar_secondary as arguments and dispatch on nonconservative_terms. That way, we can save some computations for both conservative and non-conservative systems.

Basically, to try and mimic how the P4estMesh passes this information around? I agree that this would avoid future confusion to unify the strategies across mesh types. Although this might be something better left to another PR.

amrueda · 2024-11-08T15:10:25Z

src/solvers/dgsem_tree/dg_2d.jl

-        calc_fstar!(fstar_upper, equations, surface_flux, dg, u_upper, mortar,
+        calc_fstar!(fstar_primary_upper, equations, surface_flux, dg, u_upper, mortar,
+                    orientation)
+        calc_fstar!(fstar_primary_lower, equations, surface_flux, dg, u_lower, mortar,
+                    orientation)
+        calc_fstar!(fstar_secondary_upper, equations, surface_flux, dg, u_upper, mortar,
                    orientation)
-        calc_fstar!(fstar_lower, equations, surface_flux, dg, u_lower, mortar,
+        calc_fstar!(fstar_secondary_lower, equations, surface_flux, dg, u_lower, mortar,


Same as above. I now see that we would have to pass large_sides[mortar] as an argument as well.

Hmm, good point. I need to investigate this further. The TreeMesh and how it handles the mortars is a bit cumbersome / different from how I am used to.

Okay, unifying the strategy to compute the mortar fluxes via a "smarter" calc_fstar! routine is indeed cumbersome. It would help readibility, but it is beyond this current PR (as well as my current bandwidth of projects to handle). I could open an issue and/or add such a unification to an existing mortar issue if one exists.

I'm not aware of an existing issue that is related to this. It would be great if you can create one add a link to this PR to it! 🙂

Hendrik created it

amrueda · 2024-11-08T15:17:05Z

src/solvers/dgsem_tree/dg_2d_parallel.jl

+        # Because `nonconservative_terms` is `False` the primary and secondary fluxes
+        # are identical. So, we could possibly save on computation and just pass two copies later.


I see that you also considered a different implementation with fewer calls to the flux function. This would save computations also for the non-conservative terms. Is there any reason why you decided to use this implementation?

I may have just not noticed the possible reduction in my effort to quickly throw this together. I will have a look again.

Looking again, I think I sacrificed efficiency (in this case) for parity with the non-parallel version of this routine. The mortar computations are already a bit confusing, I did not want to further increase the confusion :)

amrueda · 2024-11-08T15:26:05Z

src/solvers/dgsem_tree/dg_3d_parabolic.jl

        # Calculate fluxes
        orientation = orientations[mortar]
-        calc_fstar!(fstar_upper_left, equations_parabolic, surface_flux, dg,
-                    u_upper_left, mortar,
-                    orientation)
-        calc_fstar!(fstar_upper_right, equations_parabolic, surface_flux, dg,
-                    u_upper_right,
-                    mortar, orientation)
-        calc_fstar!(fstar_lower_left, equations_parabolic, surface_flux, dg,
-                    u_lower_left, mortar,
-                    orientation)
-        calc_fstar!(fstar_lower_right, equations_parabolic, surface_flux, dg,
-                    u_lower_right,
-                    mortar, orientation)
+        calc_fstar!(fstar_primary_upper_left, equations_parabolic, surface_flux, dg,
+                    u_upper_left, mortar, orientation)
+        calc_fstar!(fstar_primary_upper_right, equations_parabolic, surface_flux, dg,
+                    u_upper_right, mortar, orientation)
+        calc_fstar!(fstar_primary_lower_left, equations_parabolic, surface_flux, dg,
+                    u_lower_left, mortar, orientation)
+        calc_fstar!(fstar_primary_lower_right, equations_parabolic, surface_flux, dg,
+                    u_lower_right, mortar, orientation)
+        calc_fstar!(fstar_secondary_upper_left, equations_parabolic, surface_flux, dg,
+                    u_upper_left, mortar, orientation)
+        calc_fstar!(fstar_secondary_upper_right, equations_parabolic, surface_flux, dg,
+                    u_upper_right, mortar, orientation)
+        calc_fstar!(fstar_secondary_lower_left, equations_parabolic, surface_flux, dg,
+                    u_lower_left, mortar, orientation)
+        calc_fstar!(fstar_secondary_lower_right, equations_parabolic, surface_flux, dg,
+                    u_lower_right, mortar, orientation)


Since the parabolic discretization is conservative, it might be cleaner (and cheaper) to call the function four times for the primary fstar and then copy the solution to the secondary fstar?

That is a fair point to save on computation. We could take this copying strategy and just comment why it is being done.

Aha, the reason I did it this way was that there was no 3D parabolic version of the mortar_fluxes_to_elements routine that specializes on AbstractEquationsParabolic like is present for the 2D counterpart. I will throw together such a 3D routine to save on computation.

This is dealt with here (7eb20f2), right?

Yes, that was my intention. The tests are not running in CI for some reason.

amrueda

Thanks again!
I think the only missing things are:

To create an issue about the possible performance improvements.
To update the Downstream tests for Trixi2Vtk.jl

andrewwinters5000 · 2024-11-14T09:50:33Z

To create an issue about the possible performance improvements.

To update the Downstream tests for Trixi2Vtk.jl

I can create such an issue. I already have a PR open in Trixi2Vtk that updates the tests there. However, I think we need to merge this PR first and then release a new Trixi version such that the Trixi2Vtk has it available. Am I remembering correctly @ranocha ?

sloede · 2024-11-14T09:56:44Z

However, I think we need to merge this PR first and then release a new Trixi version such that the Trixi2Vtk has it available. Am I remembering correctly

Yes 👍

ranocha · 2024-11-14T14:27:39Z

I created issue #2163 to track the suggestions @amrueda made above. We are basically ready to merge this PR, @andrewwinters5000, so that the Trixi2Vtk.jl PR can be finalized, correct?

amrueda · 2024-11-14T14:43:27Z

I created issue #2163 to track the suggestions @amrueda made above. We are basically ready to merge this PR, @andrewwinters5000, so that the Trixi2Vtk.jl PR can be finalized, correct?

Thanks @ranocha .
I think everything is ready for this PR. However, some tests were failing for no apparent reason (see, e.g., https://github.com/trixi-framework/Trixi.jl/actions/runs/11838336501/job/32987230489?pr=2134#step:10:71). I just restarted them to see what happens.

ranocha · 2024-11-14T15:38:39Z

Codecov fails again. I'll merge the PR. Please keep the discussion above unresolved so that we can easily see them when working on #2163

JoshuaLampert · 2024-11-14T15:42:13Z

Regarding the failing codecov: It usually helps to create a PR from the main repo (i.e. not from a fork) if possible. These errors are usually related to tokenless upload from forks, see also #1905 and the linked issue there.

andrewwinters5000 · 2024-11-14T18:22:55Z

I created issue #2163 to track the suggestions @amrueda made above. We are basically ready to merge this PR, @andrewwinters5000, so that the Trixi2Vtk.jl PR can be finalized, correct?

Thanks for making the issue! And yes we can finalize the Trixi2Vtk PR

andrewwinters5000 added 2 commits October 29, 2024 10:52

fix P4estMesh and T8codeMesh plus tests

cca0674

apply formatter

5317195

ranocha requested a review from amrueda October 29, 2024 11:05

revert diffusion ref values. Mac values different from Ubuntu runners

218a065

amrueda reviewed Oct 29, 2024

View reviewed changes

src/solvers/dgsem_p4est/dg_2d.jl Show resolved Hide resolved

test/test_parabolic_2d.jl Outdated Show resolved Hide resolved

andrewwinters5000 changed the title ~~Fix noncons mortars for P4estMesh 2D~~ Fix noncons mortars for remaining mesh types Oct 29, 2024

andrewwinters5000 added 3 commits October 29, 2024 13:50

fix TreeMesh2D plus test

6e54391

apply formatter

37000ec

fix TreeMesh 3D and tests plus formatting

928e233

andrewwinters5000 linked an issue Oct 29, 2024 that may be closed by this pull request

Inconsistent non-conservative mortars #2130

Closed

andrewwinters5000 requested a review from amrueda October 29, 2024 14:24

andrewwinters5000 mentioned this pull request Oct 30, 2024

Update rotor reference files trixi-framework/Trixi2Vtk_reference_files#5

Merged

Merge branch 'main' into fix-noncons-mortars

b20e587

andrewwinters5000 mentioned this pull request Oct 30, 2024

Update reference commit for new rotor files trixi-framework/Trixi2Vtk.jl#99

Merged

amrueda reviewed Nov 8, 2024

View reviewed changes

andrewwinters5000 added 3 commits November 13, 2024 23:01

hopefully fix broken MPI tests

3a536f5

add specialized parabolic 3d mortar projection to save on computation

7eb20f2

Merge branch 'main' into fix-noncons-mortars

91a3d0c

andrewwinters5000 requested a review from amrueda November 13, 2024 22:05

andrewwinters5000 closed this Nov 14, 2024

andrewwinters5000 reopened this Nov 14, 2024

Merge branch 'main' into fix-noncons-mortars

c404851

amrueda approved these changes Nov 14, 2024

View reviewed changes

Merge branch 'main' into fix-noncons-mortars

b08f4ff

ranocha mentioned this pull request Nov 14, 2024

Unify and improve mortar computations #2163

Open

ranocha merged commit 07f960f into trixi-framework:main Nov 14, 2024
7 of 33 checks passed

andrewwinters5000 deleted the fix-noncons-mortars branch November 14, 2024 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix noncons mortars for remaining mesh types #2134

Fix noncons mortars for remaining mesh types #2134

andrewwinters5000 commented Oct 29, 2024 •

edited

Loading

github-actions bot commented Oct 29, 2024

codecov bot commented Oct 29, 2024 •

edited

Loading

amrueda left a comment

amrueda left a comment

amrueda Nov 8, 2024

andrewwinters5000 Nov 9, 2024

amrueda Nov 8, 2024

andrewwinters5000 Nov 9, 2024

andrewwinters5000 Nov 13, 2024

amrueda Nov 14, 2024

amrueda Nov 14, 2024

amrueda Nov 8, 2024

andrewwinters5000 Nov 9, 2024

andrewwinters5000 Nov 13, 2024

amrueda Nov 8, 2024

andrewwinters5000 Nov 9, 2024

andrewwinters5000 Nov 13, 2024

amrueda Nov 14, 2024

andrewwinters5000 Nov 14, 2024

amrueda left a comment

andrewwinters5000 commented Nov 14, 2024

sloede commented Nov 14, 2024

ranocha commented Nov 14, 2024

amrueda commented Nov 14, 2024

ranocha commented Nov 14, 2024

JoshuaLampert commented Nov 14, 2024

andrewwinters5000 commented Nov 14, 2024

		# Because `nonconservative_terms` is `False` the primary and secondary fluxes
		# are identical. So, we could possibly save on computation and just pass two copies later.

Fix noncons mortars for remaining mesh types #2134

Fix noncons mortars for remaining mesh types #2134

Conversation

andrewwinters5000 commented Oct 29, 2024 • edited Loading

github-actions bot commented Oct 29, 2024

Review checklist

Purpose and scope

Code quality

Documentation

Testing

Performance

Verification

codecov bot commented Oct 29, 2024 • edited Loading

Codecov Report

amrueda left a comment

Choose a reason for hiding this comment

amrueda left a 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

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

amrueda left a comment

Choose a reason for hiding this comment

andrewwinters5000 commented Nov 14, 2024

sloede commented Nov 14, 2024

ranocha commented Nov 14, 2024

amrueda commented Nov 14, 2024

ranocha commented Nov 14, 2024

JoshuaLampert commented Nov 14, 2024

andrewwinters5000 commented Nov 14, 2024

andrewwinters5000 commented Oct 29, 2024 •

edited

Loading

codecov bot commented Oct 29, 2024 •

edited

Loading