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

Reorganize curved elixirs in a more systematic way #892

Merged
merged 11 commits into from
Oct 5, 2021

Conversation

efaulhaber
Copy link
Member

@efaulhaber efaulhaber commented Oct 1, 2021

Together with #889, this will close #887 (I changed the advection velocity in all remaining elixirs).

However, I also did a major rework of the curved elixirs:

  • I added comments to all the elixirs that are running the same simulation as in some TreeMesh elixir, and I added comments to the test files explaining that these errors are supposed to be identical to the TreeMesh ones. So, whenever someone changes something that affects the errors of these elixirs, they (hopefully) don't just update the errors to the new ones, but actually copy the TreeMesh ones to make sure it's still giving the same results.
    I also updated examples/structured_3d_dgsem/elixir_advection_basic.jl to use the same setup as the TreeMesh variant.
  • I made the rotated and parallelogram (only for LSA) elixirs actually use the same setup as the basic ones, so they have the same nodal values in both the IC and the exact solution, but test these with non-trivial metric terms. I also added comments explaining the purpose of these elixirs and comments in the test files, again stating that the same errors as with the basic example are expected.
  • There was not a single curved LSA elixir with StructuredMesh3D with a non-constant IC. Therefore, I changed the nonperiodic LSA elixir to use a warped mesh with curved boundaries. I also changed it to use initial_condition_convergence_test instead of initial_condition_gauss, as I think that the Gauss IC is more appropriate for AMR examples.
  • There was not a single curved Euler elixir in 3D with either StructuredMesh or P4estMesh, so I changed the nonperiodic Euler elixir to use a warped mesh with curved boundaries as well.
  • Now to the P4estMesh2D. There was a nonconforming flag and a nonconforming unstructured flag. I made the unstructured one conforming, so we can test both the nonconforming and the unstructured feature isolated. The combination is still tested in the AMR unstructured flag. I also removed the nonperiodic boring Euler elixir and moved the structured nonperiodic test to the LSA extended elixir.
  • For the P4estMesh3D, I made sure that the AMR unstructured curved elixir is actually using a mesh that is FSP. Again, I removed the unnecessary nonperiodic boring Euler elixir, and I added a nonconforming unstructured curved elixir to test all the features we have with Euler (there wasn't even a curved Euler elixir for P4estMesh3D before).
  • I renamed some elixirs. For example, I added _flag to the elixirs that work on the waving flag mesh, so it's clear that these are curved elixirs.

@efaulhaber efaulhaber changed the title Revise curved elixirs in a more systematic way Reorganize curved elixirs in a more systematic way Oct 3, 2021
@efaulhaber efaulhaber marked this pull request as ready for review October 4, 2021 18:40
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.

First of all: Your high-level description sounds good to me 👍 I will review this PR soon.

Your changes reduced our coverage since some non-periodic parts for the P4estMesh3D are not tested anymore, e.g., https://coveralls.io/builds/43261839/source?filename=src%2Fmeshes%2Fp4est_mesh.jl#L212 and https://coveralls.io/builds/43261839/source?filename=src%2Fmeshes%2Fp4est_mesh.jl#L478. Could you please fix that by re-adding tests?

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.

The changes look good to me (except the coverage mentioned above)

@ranocha ranocha self-assigned this Oct 5, 2021
@efaulhaber
Copy link
Member Author

efaulhaber commented Oct 5, 2021

Thanks for the review!

I added the non-periodic Euler source terms test again for P4estMesh3D.

@ranocha ranocha enabled auto-merge (squash) October 5, 2021 07:10
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!

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, thanks for increasing consistency and improving the tests!

I left two related comments about mesh dimensions. To summarize, I think there should be tests that do not all use the same number of cells in each dimension. However, this is not something that needs to block this PR from being merged.

@codecov
Copy link

codecov bot commented Oct 5, 2021

Codecov Report

Merging #892 (d38f072) into main (9ee86eb) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #892      +/-   ##
==========================================
+ Coverage   92.71%   92.72%   +0.01%     
==========================================
  Files         229      233       +4     
  Lines       18947    18971      +24     
==========================================
+ Hits        17565    17589      +24     
  Misses       1382     1382              
Flag Coverage Δ
unittests 92.72% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...st_2d_dgsem/elixir_advection_nonconforming_flag.jl 100.00% <ø> (ø)
...est_2d_dgsem/elixir_advection_unstructured_flag.jl 100.00% <ø> (ø)
..._dgsem/elixir_advection_amr_unstructured_curved.jl 100.00% <ø> (ø)
...s/p4est_3d_dgsem/elixir_advection_nonconforming.jl 100.00% <ø> (ø)
...t_3d_dgsem/elixir_advection_unstructured_curved.jl 100.00% <ø> (ø)
...uctured_2d_dgsem/elixir_advection_parallelogram.jl 92.31% <ø> (ø)
...tructured_3d_dgsem/elixir_advection_free_stream.jl 100.00% <ø> (ø)
...s/structured_3d_dgsem/elixir_euler_source_terms.jl 100.00% <ø> (ø)
...er_source_terms_nonconforming_unstructured_flag.jl 100.00% <100.00%> (ø)
..._source_terms_nonconforming_unstructured_curved.jl 100.00% <100.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ee86eb...d38f072. Read the comment docs.

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.

Use different advection velocities in the LSA elixirs?
3 participants