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.

Sign up for GitHub

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

Jump to bottom

Initial support for surface coupling of two systems #1452

Merged
merged 94 commits into from
Jun 16, 2023
Merged

Initial support for surface coupling of two systems #1452

merged 94 commits into from
Jun 16, 2023

Conversation

SimonCan
Copy link
Contributor

@SimonCan SimonCan commented May 12, 2023 •
edited by sloede
Loading

This adds a set of boundary coupling to Trixi.jl. A full set of boundary fluxes will be added after this merge.

This PR is based on the excellent work done by @efaulhaber @bennibolm @NichtLucas in #553.

Related issue tracker: #1520

SimonCan and others added 20 commits May 11, 2023 10:35
@SimonCan
Corrected bugs from coupled to main merger.
72ff5e2
@SimonCan
Added polytropic equation.
5faea69
@SimonCan
Added further polytropic equations and examples.
17d440a
@SimonCan
Added coupling equations.
2dd9306
@SimonCan
Added coupling equation between Euler.
1ca87fb
@SimonCan
Commented debugging bits, like infiltrator.
e85ed96
@sloede
Add missing using
33b1906
@sloede
Fix destats deprecation warning
45959e1
@SimonCan
Added coupled elixir.
cfe2f8b
@SimonCan
Merge branch 'sc/coupled_to_main' of github.com:trixi-framework/Trixi…
9e8fadf 
….jl into sc/coupled_to_main
@SimonCan
Removed commented testing code.
88884d8
@SimonCan
Added other_list to the coupled semi discretisation elixir.
e7b195a
@SimonCan
Removed flux coupling equation.
47adec3
@SimonCan
Removed surface coupling equation for polytropic Euler.
4736cf2
@SimonCan
Removed polytropic Euler equation.
52abece
@SimonCan
Removed any code related to BoundaryConditionCoupledAB.
0fd0602
@SimonCan
Removed flux coupling code.
21a9af7
@SimonCan
Removed numerical fluxes for BoundaryConditionCoupledAB.
74e3280
@SimonCan
Removed surface fluxes for BoundaryConditionCoupledAB.
85ad01a
@SimonCan
Removed coupled elixir.
6023045
@SimonCan SimonCan requested a review from sloede May 12, 2023 08:26
sloede
sloede requested changes May 12, 2023
View reviewed changes
src/equations/hyperbolic_diffusion_2d.jl Outdated Show resolved Hide resolved
src/equations/hyperbolic_diffusion_2d.jl Outdated Show resolved Hide resolved
src/visualization/types.jl Outdated Show resolved Hide resolved
src/semidiscretization/semidiscretization_coupled.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_structured/dg.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_structured/dg_3d.jl Outdated Show resolved Hide resolved
ranocha
ranocha previously approved these changes Jun 11, 2023
View reviewed changes
@sloede
Copy link
Member

sloede commented Jun 11, 2023

I am slightly worried why save_solution_file for SemidiscretizationEulerGravity is not covered by tests anymore; it seems to me like there is something fishy going on:
https://coveralls.io/builds/60651096/source?filename=src%2Fsemidiscretization%2Fsemidiscretization_euler_gravity.jl#L422

My only explanation is that we accidentally implemented a more specific method that matches better than the one before, but somehow there's no complaint that something isn't working right 🤷‍♂️. Can anyone tell what's going on here?

@sloede
Copy link
Member

sloede commented Jun 11, 2023

I am slightly worried why save_solution_file for SemidiscretizationEulerGravity is not covered by tests anymore

Hopefully fixed in a69bf66.

@sloede sloede requested a review from ranocha June 11, 2023 09:13
@sloede sloede closed this Jun 11, 2023
@sloede sloede reopened this Jun 11, 2023
ranocha
ranocha previously approved these changes Jun 12, 2023
View reviewed changes
sloede
sloede previously approved these changes Jun 12, 2023
View reviewed changes
@sloede
Copy link
Member

sloede commented Jun 12, 2023 •
edited
Loading

Since this PR is heavily based on #553, we should give appropriate credit. Whoever merges this, please add the following lines to the commit message:

Co-authored-by: Benjamin Bolm [email protected]
Co-authored-by: Erik Faulhaber [email protected]
Co-authored-by: Lucas Gemein [email protected]

@bennibolm @efaulhaber @NichtLucas Please update your email addresses with your GH email address in this post here directly such that the commits are appropriately attributed to you.

ranocha
ranocha previously approved these changes Jun 12, 2023
View reviewed changes
@SimonCan
Copy link
Contributor Author

The reduced code coverage seems to be related to code that is not part of this PR. Should we then go ahead with the merge?

@ranocha
Copy link
Member

ranocha commented Jun 14, 2023

Let's wait for the formatting PR first, please

@sloede sloede dismissed stale reviews from ranocha and themself via c6276ad June 16, 2023 13:45
@sloede
Copy link
Member

sloede commented Jun 16, 2023

@SimonCan I just merged main into this and applied the formatting. I hope I did everything correct. Please have a careful look at the current diff and let me know if you think everything is ok. If yes, and if the tests all pass, I will merge this as soon as possible.

Note: It would be also great if you could also verify that the formatter did not do something ugly with your changes that we should fix before merging to main.

@SimonCan
Copy link
Contributor Author

@sloede , according to my local tests using coupling and AnalysisCallback it works. Github's tests also seem to go through.

@sloede sloede merged commit 95518c5 into main Jun 16, 2023
@sloede sloede deleted the sc/coupled-reduced branch June 16, 2023 19:35
@SimonCan SimonCan mentioned this pull request Jun 20, 2023
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sloede sloede sloede approved these changes

@ranocha ranocha ranocha left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SimonCan @sloede @ranocha