Skip to content
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

Boundary conditions on nonconservative terms not supported #1445

Closed
jlchan opened this issue May 10, 2023 · 4 comments · Fixed by #2062
Closed

Boundary conditions on nonconservative terms not supported #1445

jlchan opened this issue May 10, 2023 · 4 comments · Fixed by #2062
Labels

Comments

@jlchan
Copy link
Contributor

jlchan commented May 10, 2023

Currently, boundary conditions are not imposed on non-conservative terms. See, for example UnstructuredMesh2D:

# Compute pointwise nonconservative numerical flux at the boundary.
# In general, nonconservative fluxes can depend on both the contravariant
# vectors (normal direction) at the current node and the averaged ones.
# However, both are the same at watertight interfaces, so we pass the
# `outward_direction` twice.
# Note: This does not set any type of boundary condition for the nonconservative term
noncons_flux = nonconservative_flux(u_inner, u_inner, outward_direction, outward_direction, equations)

The implementations for DGMulti and P4estMesh are similar.

@jlchan
Copy link
Contributor Author

jlchan commented May 12, 2023

If we wanted the flexibility to set the BCs separately we would need to do something like pass a Tuple, e.g., with (boundary_condition_cons_part, boundary_condition_noncons_part).

Passing in a tuple for specialization makes sense! It'd be easy to specialize BoundaryConditionDirichlet, BoundaryConditionDoNothing, and even custom boundary conditions for this. I think this might result in a breaking change though, so we'd have to save it for 0.6.

#1431 (comment)

@sloede
Copy link
Member

sloede commented May 12, 2023

Even though it's a breaking change, it should not be a braking change. If somebody needs and implements this, we will happily bump the minor version to accommodate this.

@jlchan
Copy link
Contributor Author

jlchan commented May 12, 2023

Well done with the pun

@ranocha ranocha mentioned this issue May 13, 2023
10 tasks
@ranocha
Copy link
Member

ranocha commented May 13, 2023

Including this, there are at least two things we would like to change soon - the other one is your breaking change to the parabolic stuff. We can just do it soon, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants