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
amrueda
added a commit
to trixi-framework/TrixiAtmo.jl
that referenced
this pull request
Nov 25, 2024
…les (#47) * linear advection on the sphere * formatter * very preliminary implementation of covariant form with p4est * removed untracked * removed allocations, now 100x faster * add DS_Store to gitignore * add DS_Store to gitignore * made equations actually 2D * integrated upstream changes * analysis callback for covariant form * cleanup * separate Andres's spherical shell containers from main Trixi * Added containers and 2D p4est mesh from my spherical shell implementation in Trixi.jl (cherry-picked from f45378e) Co-authored-by: Tristan Montoya <[email protected]> * Added element container with PtrArray for performance and modified the structure * format * Fixed bug in the definition of init_elements and added nelements(). eltype() and ndims() functions for new struct * integrated Andres's new container type with covariant solver * added flux-differencing kernel for covariant form * integrated Andrés' PR with covariant solver * add tests for spherical advection in Cartesian coords * revert change to moist bubble case * revert change to moist bubble case * covariant weak form with tests * removed shallow water from this branch * reorganize a bit * hard-code dimension of node coordinates to avoid type instability * metric terms for shell of radius != 1 * test for covariant form is now on earth scale * fix name of cartesian test * moved i,j,element,cache into flux signatures for covariant form * added test for flux differencing covariant form * transformation between local coordinate systems now uses latitude-longitude as intermediate state, not Cartesian * format * make compatible with my PR trixi-framework/Trixi.jl#2068 * separate containers to store geometric information specific to covariant form * add element-local mapping from Guba et al. * add reference to Guba et al. to docstring * new mapping works with cartesian solver solver in covariant_form branch * exact metrics for element-local mapping * streamline tests and improve readability * rename default DGSEM mapping * didn't need new analysis cache * improve comments * refactor * remove DiffEqCallbacks for now * add better comments * improve comments * better documentation * add NDIMS_AMBIENT parameter to equations * fix docstring format * make link in docs work * fix link in docs * documentation for the covariant solver * added surface central flux and test * fix trixi_compat * specialize compute_coefficients! to dimension 2 * format * fix spelling * remove initial_condition parameter from rhs * no need to use Float32 for physical constant defaults * incorporate downstream changes * format * standardize test cases * move covariant form max_dt method to callbacks_step * Fix comments * generalize interface between equations and cache * relax relative tolerance on cartesian advection test) * improve docs * streamline initial condition definition and improve docs * tests for cartesian form now use standard and element-local mapping approaches * first attempt at aux vars * works with allocations * fix allocations * clean up docs * format and update comments * move auxvars into own cache field * improve comments * format comments * revert back to without basis matrix inverse * add warning for P4estMeshCubedSphere and rename volume->area element * rename function transforming initial condition * fix elixir and docs * poldeg(solver) -> Trixi.polydeg(solver) * reformat ugly line breaks * modularize covariant basis calculation for easier testing * remove duplicated cons2entropy function --------- Co-authored-by: Andrés Rueda-Ramírez <[email protected]>
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.