-
Notifications
You must be signed in to change notification settings - Fork 109
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
Initial support for P4estMesh #593
Conversation
* Add P4estMesh and implement unstructured calc_interface_flux! * Comment indexfunction_reduced * Save cell geometry in interpolation nodes instead of a mapping function * Use P4est to compute node coordinates * Extract interface information from p4est * Make indexfunction_surface type stable * Downgrade P4est_jll to 2.2 * Add P4est_jll as dependency * Update Project.toml * Update Project.toml * Fix node_coordinates interpolation * Add kwarg initial_refinement_level * Add documentation of P4estMesh * Remove trees_per_dimension to make P4estMesh truly unstructured * Add proper show function * Remove P4est_jll from dependencies * Improve comments * Destroy p4est data structures on exit * Add test case for P4estMesh * Implement suggestions * Implement suggestions * Revise constructor of P4estMesh to use kwargs * Improve performance of indices2direction * Fix mesh polydegs and stepsize for non-constant speeds * Implement suggestions
@efaulhaber I think you need to replace |
Yes, I already did this while working on the BCs. |
* Replace @timeit_debug with @timed * Implement save/restart for P4estMesh * Return node_coordinates * Change parameter basis to nodes to reuse function in Trixi2Vtk * Add Trixi2Vtk dev tips to docs * Fix docs * Update docs/src/visualization.md Co-authored-by: Michael Schlottke-Lakemper <[email protected]> * Update src/mesh/mesh_io.jl Co-authored-by: Michael Schlottke-Lakemper <[email protected]> * Implement suggestions Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
* Replace @timeit_debug with @timed * Extract orientation from face info * Reuse code from UnstructuredQuadMesh to implement non-periodic boundaries * Fix save/restart * Add nonperiodic example * Add assertion * Remove periodicity from P4estMesh * Fix loading meshes from different paths * Add constructor to build P4estMeshes from ABAQUS files * Allow initial_refinement_level > 1 * Add mapping as additional parameter to P4estMesh from file * Rename calc_node_coordinates! to avoid ambiguity * Add curved p4est Euler FSP example * Add documentation to new constructor * Check BCs for integrity * Add EOC test for non-periodic Euler on unstructured mesh * Implement suggestions * Implement suggestions * Fix tests * Implement suggestions * Fix spacing
Codecov Report
@@ Coverage Diff @@
## main #593 +/- ##
=======================================
Coverage ? 93.36%
=======================================
Files ? 155
Lines ? 15353
Branches ? 0
=======================================
Hits ? 14334
Misses ? 1019
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
* Prepare data structures for non-conforming meshes * Implement prolong2mortars * Implement mortar flux (not working yet) * Fix mortar flux calculation * Use global number of quadrants * Include tests for P4estMesh * Implement suggestions * Add test for non-conforming P4estMesh * Use Downloads.download * Implement suggestions * Fix tests * Fix CI tests on macOS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a test where you use a different polynomial degree for the solver and the mesh? If not, could you please add such a test?
* Implement AMR with p4est (needs debugging) * Fix TreeMesh AMR * Fix print_amr_information * Fix refinement * Fix CurvedMesh * Fix AMR with p4est * Add general print_amr_information for non-AMR meshes * Fix boundary conditions with AMR * Fix AMR on unstructured meshes * Add tests for AMR with p4est * Use Downloads: download * Improve performance of P4estMesh * Prepare PR * Fix 2279806 * Remove some allocations from count_required functions * Remove more allocations * Implement suggestions * Implement more suggestions * Fix polydeg in show(P4estMesh) * Destroy p4est data structures in inner constructor * Update examples/2d/elixir_advection_amr_p4est_unstructured_flag.jl Co-authored-by: Hendrik Ranocha <[email protected]> * Update examples/2d/elixir_advection_p4est_non_conforming_flag.jl Co-authored-by: Hendrik Ranocha <[email protected]> * Make AMR controllers independent of the mesh * Implement suggestions * Fix e71913f * Implement suggestions * Fix 6a6a183 Co-authored-by: Hendrik Ranocha <[email protected]>
We should be ready to merge this once
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@efaulhaber @ranocha I have two small questions, but after that, I think we can merge it once the tests pass and coverage is ok
@@ -22,6 +22,9 @@ const EXAMPLES_DIR = joinpath(pathof(Trixi) |> dirname |> dirname, "examples") | |||
mean_eoc = convergence_test(@__MODULE__, joinpath(EXAMPLES_DIR, "2d", "elixir_advection_extended_curved.jl"), 3) | |||
@test isapprox(mean_eoc[:l2], [4.0], rtol=0.01) | |||
|
|||
mean_eoc = convergence_test(@__MODULE__, joinpath(EXAMPLES_DIR, "2d", "elixir_euler_nonperiodic_p4est.jl"), 3) | |||
@test isapprox(mean_eoc[:l2], [3.54, 3.50, 3.50, 3.52], rtol=0.01) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@efaulhaber Can you tell me agian why the EOCs are so low?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewwinters5000 could we get your take on this? Such a mesh with straight sides should give us the full EOC, shouldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are lots of kite-like elements (when one of the angles is very obtuse and the element is approaching a triangle) in this mesh which can mess with the quality of the EOCs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the EOC look in the last few refinement levels? Sometimes the mean EOC can get polluted with sub-optimal values from too coarse resolution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something is fishy here...does this use calc_interface_flux!
from dg_unstructured
or did you write a new one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote a new one, but it's very similar to yours.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I am still trying to track down the EOCs issue in my implementation (so it could have a bug in it). I was thinking that the problem was the mapping, but your EOCs seem to indicate that it is also a problem for straight-sided elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you convert my .inp
mesh file to one that you can read somehow? Then you could use the same mesh with your solver (maybe even make it periodic?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the second (less crazy) mesh.
All tests have passed, but coverage is still not optimal. Do you see an easy way to increase test coverage by looking at the "New missed lines in diff" at https://coveralls.io/builds/40470257? Maybe by removing something that is not necessary or by slightly modifying an existing test? OTOH, the difference (-0.03%) is not so big as to require fundamental changes with a big effort, but it would be good to check for some low-hanging fruits. |
Most of these are the "passive solver" stuff in
is now unused. Do you want me to remove this line in a new PR? I used to test if the numbers of boundaries in the types container matches the number of boundaries of the boundary container, but this is not correct. Adding two boundaries on the left and removing two on the right would still require the container to be reinitialized. |
Have you tested whether it works? If not, I'd rather not have it in there but maybe an error message somewhere in the beginning saying that this has not been implemented or tested yet. There's also a bunch of untested lines here and it seems like there is no test for the "structured" P4est ctor for non-periodic cells (see here). The former seems like it can be just removed, while for the latter I think it would be good to have it tested in case someone would like to use it - it seems like a reasonable use case. |
Oops, I missed that. I'll add a test for a non-periodic structured mesh. |
Alright, Euler gravity works with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
No description provided.