Add type parameter NDIMS_AMBIENT to P4estMesh for dimension of ambient space
#2068
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Review checklistThis checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging. Purpose and scope
Code quality
Documentation
Testing
Performance
Verification
Created with ❤️ by the Trixi.jl community. |
tristanmontoya
changed the title
P4estMesh for dimension of ambient space P4estMesh for dimension of ambient space
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2068 +/- ##
=======================================
Coverage 96.32% 96.32%
=======================================
Files 470 470
Lines 37447 37467 +20
=======================================
+ Hits 36070 36090 +20
Misses 1377 1377
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
tristanmontoya
changed the title
P4estMesh for dimension of ambient space NDIMS_AMBIENT to P4estMesh for dimension of ambient space
tristanmontoya
changed the title
NDIMS_AMBIENT to P4estMesh for dimension of ambient space NDIMS_AMBIENT to P4estMesh for dimension of ambient space
tristanmontoya
added a commit
to trixi-framework/TrixiAtmo.jl
that referenced
this pull request
Sep 7, 2024
DanielDoehring
requested changes
Sep 10, 2024
DanielDoehring
requested changes
Sep 10, 2024
ranocha
reviewed
Sep 11, 2024
ranocha
approved these changes
Sep 11, 2024
There was a problem hiding this comment.
Thanks. This is fine with me and can be merged when @DanielDoehring approves it, too
DanielDoehring
approved these changes
Sep 11, 2024
tristanmontoya
added a commit
to trixi-framework/TrixiAtmo.jl
that referenced
this pull request
Sep 12, 2024
…xi.jl (#35) * make compatible with my PR trixi-framework/Trixi.jl#2068 * update init_elements to use new P4estMesh type parameter
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR supersedes #2046 in order to facilitate the solution of PDEs on two-dimensional manifolds in three-dimensional space using TrixiAtmo.jl and the visualization of such solutions using trixi-framework/Trixi2Vtk.jl#82.
With the proposed change,
P4estMesh{NDIMS, NDIMS_AMBIENT}represents a tessellation of a manifold of dimensionNDIMSdefined within an ambient space of dimensionNDIMS_AMBIENT. The parameterNDIMS_AMBIENTis set within the inner constructor ofP4estMeshtosize(tree_node_coordinates, 1).Note that I have added
NDIMS_AMBIENTas the second type parameter, so all uses ofP4estMesh{NDIMS, RealT}andP4estMesh{NDIMS, RealT, IsParallel}are no longer valid, and have been replaced accordingly in this PR. All methods dispatching on such types should be specialized toP4estMesh{NDIMS, NDIMS, ... }if one does not intend to allow manifolds embedded in higher dimensional spaces. For example, this would affect @SimonCan's PR #1761 on this line here.While
calc_node_coordinates!andcreate_cache_analysishave been updated in both 2D and 3D to allow forNDIMS != NDIMS_AMBIENT, I have specializedinit_elementsto requireP4estMesh{NDIMS, NDIMS}. This is because TrixiAtmo.jl defines its own containers usingPtrArrayand computes specialized metric terms for the spherical shell usingP4estMesh{2, 3}, which we wanted to keep separate from the main Trixi.jl package. So, with this PR, the user is free to define a mesh usingP4estMesh{NDIMS, NDIMS_AMBIENT}withNDIMS != NDIMS_AMBIENT, but they would have to define their owninit_elementsto specialize the metric terms for the desired application, as we do in TrixiAtmo.jl. At some point, we could consider generalizinginit_elementsand the associated containers to allow for the treatment of arbitrary manifolds (in which case, the functionality could be merged into Trixi.jl, rather than TrixiAtmo), but I have taken the YAGNI approach here and deferred that to future work.Although I have labelled this as a breaking change, the vast majority of methods using
P4estMeshdispatch only on the first type parameter,NDIMS, and this PR does not affect such uses. Therefore, the typical user of Trixi.jl can continue to useP4estMesh{NDIMS}without having to think aboutNDIMS_AMBIENTat all. Because of this, I have chosen to keep the docstring asP4estMesh{NDIMS}, although I am open to changing this if we decide to document this feature as part of Trixi's public API.Regarding performance, the type instability from #2046 is now gone, and we get similar runtimes as before when calling
calc_node_coordinates!. To see this, let's run the following code:With the current
main, we get the following benchmarks:With the proposed changes, the performance is similar:
Running a more complete case,
examples/p4est_2d_dgsem/elixir_euler_blast_wave_amr.jl, usingjulia --threads 1on my MacBook Air M1, we also see no significant change. With the currentmain, we get the following:With the new code:
I would be very grateful for any feedback/comments on these changes or suggestions of further tests to run.