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

AMR for 2D Parabolic P4est #1691

Merged
merged 153 commits into from
Nov 21, 2023

Conversation

DanielDoehring
Copy link
Contributor

@DanielDoehring DanielDoehring commented Oct 25, 2023

This aims to bundle the implementation of parabolic P4est mortars from @jlchan and @apey236 (see #1662) into a recent version of the main repository.

Related to #1147, #1674

Copy link
Contributor

@jlchan jlchan left a comment

Choose a reason for hiding this comment

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

Thanks for porting this PR over and cleaning it up @DanielDoehring! I've added some comment suggestions but otherwise LGTM.

Since I was involved with the original version of this PR, someone else should take a brief look at this as well, at least at the src files.

src/callbacks_step/amr.jl Show resolved Hide resolved
src/callbacks_step/amr.jl Show resolved Hide resolved
src/solvers/dgsem_p4est/dg_2d_parabolic.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_p4est/dg_2d_parabolic.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_p4est/dg_2d_parabolic.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_p4est/dg_2d_parabolic.jl Outdated Show resolved Hide resolved
@jlchan
Copy link
Contributor

jlchan commented Nov 16, 2023

I was involved in writing the original version of this PR, so I probably shouldn't be the only reviewer.

@andrewwinters5000 @efaulhaber would either of you be able to take a brief look at the src changes?

@DanielDoehring
Copy link
Contributor Author

Thanks a lot @jlchan for the valuable comments! Those will sure come in handy to explain the approach taken and remind ourselves in the future :D

jlchan
jlchan previously approved these changes Nov 17, 2023
@jlchan
Copy link
Contributor

jlchan commented Nov 17, 2023

Can you fix the failing formatting tests?

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.

I'm not familiar with the parabolic stuff, and it's been a while since I've written the P4estMesh code (and even longer since I've worked with the AMR parts of Trixi), so I can't really give a good review here without spending too much time.

But it does look like a lot of that code is very similar to existing code in Trixi, so it might be possible to avoid some duplicate code.

src/callbacks_step/amr.jl Outdated Show resolved Hide resolved
src/callbacks_step/amr.jl Show resolved Hide resolved
@jlchan
Copy link
Contributor

jlchan commented Nov 20, 2023

@ranocha @DanielDoehring the only thing failing is the coverage test. It looks like some of it is related to coarsening. Is there a suggested way to test this?

@DanielDoehring
Copy link
Contributor Author

@ranocha @DanielDoehring the only thing failing is the coverage test. It looks like some of it is related to coarsening. Is there a suggested way to test this?

Not sure what is going on there. The part marked in red is definitely being called (I even added a println statement to be sure) in the test "P4estMesh2D: elixir_advection_diffusion_nonperiodic_amr.jl":

@jlchan
Copy link
Contributor

jlchan commented Nov 20, 2023

@ranocha @DanielDoehring the only thing failing is the coverage test. It looks like some of it is related to coarsening. Is there a suggested way to test this?

Not sure what is going on there. The part marked in red is definitely being called (I even added a println statement to be sure) in the test "P4estMesh2D: elixir_advection_diffusion_nonperiodic_amr.jl":

Ah, maybe it's Codecov being weird then.

@ranocha @sloede both Erik and I reviewed this PR. Tests pass except for Codecov, which might be just Codecov weirdness. Are you ok if we merge this?

Copy link
Member

@ranocha ranocha left a comment

Choose a reason for hiding this comment

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

Thanks!

@ranocha ranocha enabled auto-merge (squash) November 21, 2023 06:38
@ranocha ranocha merged commit 3c1762b into trixi-framework:main Nov 21, 2023
31 of 33 checks passed
@DanielDoehring DanielDoehring deleted the AMR_Parabolic_P4est branch November 21, 2023 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants