Add flexibility to nonconservative BCs #2200

MarcoArtiano · 2024-12-10T15:26:16Z

This PR allows to have more flexibility when defining the boundary conditions for a set of equations with nonconservative terms. This issues has been discussed in #2175. Now the user can define his own boundary condition, considering that the function accepts both surface flux and the nonconservative flux. An example can be found in elixir_mhd_reflective_wall

github-actions · 2024-12-10T15:26:32Z

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-12-10T15:59:23Z

Codecov Report

Attention: Patch coverage is 92.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 96.41%. Comparing base (a1be62b) to head (53d8a14).
Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
src/basic_types.jl	50.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2200      +/-   ##
==========================================
- Coverage   96.42%   96.41%   -0.00%     
==========================================
  Files         486      489       +3     
  Lines       39164    39398     +234     
==========================================
+ Hits        37761    37985     +224     
- Misses       1403     1413      +10

Flag	Coverage Δ
unittests	`96.41% <92.50%> (-<0.01%)`	⬇️

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.

MarcoArtiano · 2024-12-11T09:38:15Z

According to the way this feature has been implemented, the functions boundary_condition_slip_wall in shallow_water_wet_dry_1d.jl and shallow_water_wet_dry_2d.jl should have been modified accordingly.

CC @andrewwinters5000

andrewwinters5000

Thanks for tackling this @MarcoArtiano! The functionality works as discussed where it is on the user to put in the boundary conditions for the conservative and nonconservative terms and then combine them appropriately, i.e., scaling noncons with 0.5f0. This is the only aspect I am not a huge fan of, as it is related to how the nonconservative terms are implemented at interfaces and has nothing to do with boundary conditions.

Because of this, we should maybe document where this factor of 0.5f0 magically comes from in the new boundary condition routines (as it was documented beofre inside the compute boundary flux call).

What do you think @patrickersing ?

andrewwinters5000 · 2024-12-18T09:01:19Z

src/solvers/dgsem_p4est/dg_2d.jl

        # Note the factor 0.5 necessary for the nonconservative fluxes based on
        # the interpretation of global SBP operators coupled discontinuously via
        # central fluxes/SATs


This comment would possibly need added to where the factor of 0.5 scaling occurs.

I am also not very content that we have to expose this to the user in order to specify the boundary condition, but I don't see a better way to accomplish this. I definitely think that we should comment this to give some explanation where the factor 0.5 comes from and that it is not specific to any boundary condition.

I agree with all of you. I'm also not a huge fan of the factor 0.5. We may alternatively return a tuple from the boundary conditions for non-conservative systems and multiply by 0.5 inside the solver. As an example:

flux_, noncons_ = boundary_condition(u_inner, normal_direction, x, t, surface_integral.surface_flux, equations) # Copy flux to element storage in the correct orientation for v in eachvariable(equations) # Note the factor 0.5 necessary for the nonconservative fluxes based on # the interpretation of global SBP operators coupled discontinuously via # central fluxes/SATs surface_flux_values[v, node_index, direction_index, element_index] = flux_[v] + 0.5f0 * noncons_[v] end

What do you think?

I like this idea, since it doesn't require knowledge about the implementation aspect of adding the 0.5 to create the boundary condition. It should work well for the BCs that are currently implemented, but I am not sure what this would look like for the new type of boundary condition that you plan to add. Would you then only set the flux_ part and set the noncons_ part to zero?
I think it would be helpful to include a specific example with such a new BC in the PR to better understand and evaluate what the implementation should look like.

I also like this idea with the documentation of how it would work in practice from Patrick's comment above. That is, for something like the standard shallow water equations the jump in the bottom topography is zero at the physical boundary (typically) so one's new boundary condition could compute the conservative flux pieces and then have

return flux_, SVector(0,0,0,0) # flux, noncons

DanielDoehring

Thanks for this! The only thing that I suggest is using the little more verbose noncons_flux instead of noncons.

src/solvers/dgsem_p4est/dg_2d.jl

src/equations/shallow_water_2d.jl

examples/dgmulti_2d/elixir_mhd_reflective_wall.jl

src/equations/equations.jl

Co-authored-by: Daniel Doehring <[email protected]>

MarcoArtiano · 2025-01-08T11:50:56Z

Thanks for this! The only thing that I suggest is using the little more verbose noncons_flux instead of noncons.

Thanks! You are definitely right! I committed all your changes.

src/solvers/dgsem_p4est/dg_2d.jl

andrewwinters5000

Thanks @MarcoArtiano ! I like this structure and it is quite clean to have the boundary_condition return both fluxes a la

    flux, noncons_flux = boundary_condition(u_inner, outward_direction, x, t,
                                            surface_integral.surface_flux, equations)

I just left a few minor suggestions.

src/equations/shallow_water_1d.jl

src/equations/shallow_water_2d.jl

Co-authored-by: Andrew Winters <[email protected]>

patrickersing

Thanks @MarcoArtiano! I think the PR is looking good. I just have some smaller suggestions.

patrickersing · 2025-01-24T12:54:34Z

src/basic_types.jl

@@ -74,13 +74,37 @@ struct BoundaryConditionDoNothing end
    return surface_flux(u_inner, u_inner, orientation_or_normal_direction, equations)


Suggested change

return surface_flux(u_inner, u_inner, orientation_or_normal_direction, equations)

return flux(u_inner, orientation_or_normal_direction, equations)

In a previous PR (https://github.com/trixi-framework/Trixi.jl/pull/2062/files#r1768020588) I changed this from flux to surface_flux. such that the BC works for both conservative and nonconservative systems. Now that we treat them separately, we should be able to only use the flux-function in the conservative case.

patrickersing · 2025-01-24T12:55:37Z

src/basic_types.jl

+           nonconservative_flux_function(u_inner, u_inner,
+                                         orientation_or_normal_direction, equations)
+end
+
 # This version can be called by hyperbolic solvers on unstructured, curved meshes
 @inline function (::BoundaryConditionDoNothing)(u_inner,
                                                outward_direction::AbstractVector,
                                                x, t, surface_flux, equations)
    return surface_flux(u_inner, u_inner, outward_direction, equations)


Suggested change

return surface_flux(u_inner, u_inner, outward_direction, equations)

return flux(u_inner, outward_direction, equations)

see comment above

patrickersing · 2025-01-24T13:24:28Z

test/test_type.jl

@@ -2258,7 +2258,7 @@ isdir(outdir) && rm(outdir, recursive = true)
            directions = [1, 2]
            normal_direction = SVector(one(RealT))

-            surface_flux_function = flux_lax_friedrichs
+            surface_flux_function = (flux_lax_friedrichs, flux_lax_friedrichs)


Shouldn't we set a true nonconservative flux here instead of flux_lax_friedrichs?

patrickersing · 2025-01-24T13:24:52Z

test/test_type.jl

@@ -2347,7 +2348,7 @@ isdir(outdir) && rm(outdir, recursive = true)
            directions = [1, 2, 3, 4]
            normal_direction = SVector(one(RealT), zero(RealT))

-            surface_flux_function = flux_lax_friedrichs
+            surface_flux_function = (flux_lax_friedrichs, flux_lax_friedrichs)


See comment above

patrickersing · 2025-01-24T13:35:35Z

src/solvers/dgsem_p4est/dg_2d.jl

    # Call pointwise numerical flux function for the conservative part
    # in the normal direction on the boundary


Suggested change

# Call pointwise numerical flux function for the conservative part

# in the normal direction on the boundary

# Call pointwise numerical flux functions for the conservative and nonconservative part

# in the normal direction on the boundary

patrickersing · 2025-01-24T13:36:30Z

src/solvers/dgsem_unstructured/dg_2d.jl

    # Call pointwise numerical flux function for the conservative part
    # in the normal direction on the boundary


Suggested change

# Call pointwise numerical flux function for the conservative part

# in the normal direction on the boundary

# Call pointwise numerical flux functions for the conservative and nonconservative part

# in the normal direction on the boundary

patrickersing · 2025-01-24T13:37:13Z

src/solvers/dgsem_unstructured/dg_2d.jl


    # Compute pointwise nonconservative numerical flux at the boundary.


Suggested change

# Compute pointwise nonconservative numerical flux at the boundary.

This comment can be removed.

MarcoArtiano added 4 commits December 10, 2024 13:34

adding flexibility to noncons BCs

e2e2c20

minor fix

1e925fc

Dirichlet BC for noncons

31f1e0a

formatting

eb1ccdd

MarcoArtiano added 3 commits December 10, 2024 17:45

Noncons Dirichlet BC

e4bc181

test_type.jl fix

8c5d2fc

dg multi fix

c2b44ae

MarcoArtiano changed the title ~~Add flexibility to nonconservative BCs~~ WIP: Add flexibility to nonconservative BCs Dec 11, 2024

DanielDoehring added the breaking label Dec 17, 2024

andrewwinters5000 reviewed Dec 18, 2024

View reviewed changes

moving 0.5 factor internally

b60eaf1

MarcoArtiano marked this pull request as ready for review January 7, 2025 12:21

Merge branch 'main' into noncons

d7dce5e

DanielDoehring requested changes Jan 8, 2025

View reviewed changes

MarcoArtiano and others added 13 commits January 8, 2025 12:48

Update src/solvers/dgsem_p4est/dg_2d.jl

840fa9e

Co-authored-by: Daniel Doehring <[email protected]>

Update src/equations/shallow_water_2d.jl

2baa52a

Co-authored-by: Daniel Doehring <[email protected]>

Update src/equations/shallow_water_2d.jl

cd2186b

Co-authored-by: Daniel Doehring <[email protected]>

Update src/equations/equations.jl

7a326fe

Co-authored-by: Daniel Doehring <[email protected]>

Update examples/dgmulti_2d/elixir_mhd_reflective_wall.jl

f9b19c3

Co-authored-by: Daniel Doehring <[email protected]>

Update src/equations/shallow_water_2d.jl

c35cff9

Co-authored-by: Daniel Doehring <[email protected]>

Update src/equations/shallow_water_2d.jl

26a756a

Co-authored-by: Daniel Doehring <[email protected]>

Update src/equations/shallow_water_2d.jl

307197d

Co-authored-by: Daniel Doehring <[email protected]>

Update src/equations/equations.jl

8a1a3d5

Co-authored-by: Daniel Doehring <[email protected]>

Update src/equations/equations.jl

5068717

Co-authored-by: Daniel Doehring <[email protected]>

Update src/equations/equations.jl

06b881e

Co-authored-by: Daniel Doehring <[email protected]>

Update src/equations/shallow_water_1d.jl

4d112a4

Co-authored-by: Daniel Doehring <[email protected]>

Update src/equations/shallow_water_1d.jl

ecaf306

Co-authored-by: Daniel Doehring <[email protected]>

Update src/equations/shallow_water_1d.jl

1bd0184

Co-authored-by: Daniel Doehring <[email protected]>

DanielDoehring reviewed Jan 8, 2025

View reviewed changes

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

MarcoArtiano added 2 commits January 8, 2025 13:34

minor fixes

51505aa

fix test type stability

d26925e

DanielDoehring previously approved these changes Jan 8, 2025

View reviewed changes

fix test type stability

9b4e384

MarcoArtiano dismissed DanielDoehring’s stale review via 9b4e384 January 9, 2025 08:38

MarcoArtiano changed the title ~~WIP: Add flexibility to nonconservative BCs~~ Add flexibility to nonconservative BCs Jan 9, 2025

MarcoArtiano mentioned this pull request Jan 9, 2025

WIP: Non-Conservative BCs update trixi-framework/TrixiShallowWater.jl#64

Open

DanielDoehring added refactoring Refactoring code without functional changes enhancement New feature or request labels Jan 14, 2025

DanielDoehring mentioned this pull request Dec 2, 2024

Release v0.10 #2105

Open

9 tasks

andrewwinters5000 requested changes Jan 24, 2025

View reviewed changes

src/equations/shallow_water_1d.jl Show resolved Hide resolved

src/equations/shallow_water_2d.jl Show resolved Hide resolved

MarcoArtiano and others added 2 commits January 24, 2025 09:15

Update src/equations/shallow_water_1d.jl

8332bbe

Co-authored-by: Andrew Winters <[email protected]>

Update src/equations/shallow_water_2d.jl

53d8a14

Co-authored-by: Andrew Winters <[email protected]>

patrickersing reviewed Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flexibility to nonconservative BCs #2200

Add flexibility to nonconservative BCs #2200

MarcoArtiano commented Dec 10, 2024 •

edited

Loading

github-actions bot commented Dec 10, 2024

codecov bot commented Dec 10, 2024 •

edited

Loading

MarcoArtiano commented Dec 11, 2024 •

edited

Loading

andrewwinters5000 left a comment

andrewwinters5000 Dec 18, 2024

patrickersing Dec 18, 2024

MarcoArtiano Dec 18, 2024 •

edited

Loading

patrickersing Dec 18, 2024

andrewwinters5000 Dec 19, 2024

DanielDoehring left a comment

MarcoArtiano commented Jan 8, 2025

andrewwinters5000 left a comment

patrickersing left a comment

patrickersing Jan 24, 2025

patrickersing Jan 24, 2025

patrickersing Jan 24, 2025

patrickersing Jan 24, 2025

patrickersing Jan 24, 2025

patrickersing Jan 24, 2025

patrickersing Jan 24, 2025

		@@ -74,13 +74,37 @@ struct BoundaryConditionDoNothing end
		return surface_flux(u_inner, u_inner, orientation_or_normal_direction, equations)

	return surface_flux(u_inner, u_inner, orientation_or_normal_direction, equations)
	return flux(u_inner, orientation_or_normal_direction, equations)

	return surface_flux(u_inner, u_inner, outward_direction, equations)
	return flux(u_inner, outward_direction, equations)

		# Call pointwise numerical flux function for the conservative part
		# in the normal direction on the boundary


		# Compute pointwise nonconservative numerical flux at the boundary.

Add flexibility to nonconservative BCs #2200

Are you sure you want to change the base?

Add flexibility to nonconservative BCs #2200

Conversation

MarcoArtiano commented Dec 10, 2024 • edited Loading

github-actions bot commented Dec 10, 2024

Review checklist

Purpose and scope

Code quality

Documentation

Testing

Performance

Verification

codecov bot commented Dec 10, 2024 • edited Loading

Codecov Report

MarcoArtiano commented Dec 11, 2024 • edited Loading

andrewwinters5000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoArtiano Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielDoehring left a comment

Choose a reason for hiding this comment

MarcoArtiano commented Jan 8, 2025

andrewwinters5000 left a comment

Choose a reason for hiding this comment

patrickersing 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

MarcoArtiano commented Dec 10, 2024 •

edited

Loading

codecov bot commented Dec 10, 2024 •

edited

Loading

MarcoArtiano commented Dec 11, 2024 •

edited

Loading

MarcoArtiano Dec 18, 2024 •

edited

Loading