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

Add parabolic terms for P4estMesh{2} #1490

Merged
merged 42 commits into from
Jun 2, 2023

Conversation

jlchan
Copy link
Contributor

@jlchan jlchan commented May 25, 2023

This PR implements (periodic) parabolic terms on curved 2D P4est meshes. There is currently a test for advection-diffusion. We haven't yet added a Navier-Stokes test because elixir_navierstokes_convergence.jl requires boundary conditions.

#1492 should be merged first to reduce the size of this PR. Additionally, boundary conditions will be implemented separately in #1493 to reduce the size of each PR.

@codecov
Copy link

codecov bot commented May 26, 2023

Codecov Report

Merging #1490 (2b91590) into main (4f35801) will decrease coverage by 0.39%.
The diff coverage is 90.53%.

@@            Coverage Diff             @@
##             main    #1490      +/-   ##
==========================================
- Coverage   96.14%   95.75%   -0.39%     
==========================================
  Files         360      363       +3     
  Lines       29945    30183     +238     
==========================================
+ Hits        28788    28900     +112     
- Misses       1157     1283     +126     
Flag Coverage Δ
unittests 95.75% <90.53%> (-0.39%) ⬇️

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

Impacted Files Coverage Δ
src/solvers/dgsem_p4est/dg.jl 100.00% <ø> (ø)
src/solvers/dgsem_p4est/dg_2d_parabolic.jl 88.83% <88.83%> (ø)
...st_2d_dgsem/elixir_advection_diffusion_periodic.jl 100.00% <100.00%> (ø)
...gsem/elixir_advection_diffusion_periodic_curved.jl 100.00% <100.00%> (ø)
src/solvers/dgsem_tree/dg_2d_parabolic.jl 94.92% <100.00%> (ø)

... and 6 files with indirect coverage changes

@jlchan jlchan marked this pull request as ready for review May 26, 2023 20:45
@jlchan jlchan requested a review from ranocha May 26, 2023 20:47
jlchan and others added 3 commits June 1, 2023 09:08
@jlchan
Copy link
Contributor Author

jlchan commented Jun 1, 2023

Thanks @sloede and @efaulhaber for the reviews! I've addressed the points raised.

@jlchan jlchan requested review from sloede and efaulhaber June 1, 2023 14:50
@jlchan
Copy link
Contributor Author

jlchan commented Jun 1, 2023

Oops, didn't see this comment @sloede

I haven't done a check of the implementation logic but only looked at the syntax (which looks fine). Have you been able to show the expected EOC when running convergence tests with both new elixirs?

I haven't tested this yet, but I plan to add it in the boundary condition PR.

Also, coverage is not optimal since the boundary condition code is mostly unused - am I right to assume that this will be added in a subsequent PR?

Yep, in #1493

sloede
sloede previously approved these changes Jun 1, 2023
Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (but please wait for @efaulhaber for the 🟢 )

@jlchan jlchan requested a review from efaulhaber June 1, 2023 14:57
efaulhaber
efaulhaber previously approved these changes Jun 1, 2023
Copy link
Member

@efaulhaber efaulhaber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looking good now. Math makes sense 👍

@jlchan jlchan requested a review from sloede June 1, 2023 15:53
@jlchan
Copy link
Contributor Author

jlchan commented Jun 1, 2023

@sloede your old review was wiped when I added some commits. Mind approving again?

@efaulhaber
Copy link
Member

Or does it make sense to add a comment to that part? Maybe just a reference to the corresponding discussion here, where we explained everything in proper formulas?

@jlchan
Copy link
Contributor Author

jlchan commented Jun 1, 2023

Or does it make sense to add a comment to that part? Maybe just a reference to the corresponding discussion here, where we explained everything in proper formulas?

Good idea. Added in 8956587

@jlchan jlchan requested a review from efaulhaber June 1, 2023 15:57
@sloede sloede enabled auto-merge (squash) June 1, 2023 16:00
@jlchan
Copy link
Contributor Author

jlchan commented Jun 2, 2023

The coverage drops prevent this from merging. The drop in coverage is from the boundary condition stuff, which we'll add in #1493.

@ranocha
Copy link
Member

ranocha commented Jun 2, 2023

Ok

@ranocha ranocha merged commit 6bb298d into trixi-framework:main Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants