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

Feature matrix #611

Closed
ranocha opened this issue May 25, 2021 · 19 comments · Fixed by #703
Closed

Feature matrix #611

ranocha opened this issue May 25, 2021 · 19 comments · Fixed by #703
Assignees
Labels
documentation Improvements or additions to documentation

Comments

@ranocha
Copy link
Member

ranocha commented May 25, 2021

As discussed today, it would be great to have a feature matrix of the meshes/solvers/whatever stuff in the docs.

@ranocha ranocha added the documentation Improvements or additions to documentation label May 25, 2021
@ranocha
Copy link
Member Author

ranocha commented Jul 7, 2021

Here is a first second third suggestion how such a feature matrix might look like.

Feature TreeMesh StructuredMesh UnstructuredMesh2D P4estMesh
Spatial dimension 1D, 2D, 3D 1D, 2D, 3D 2D 2D, 3D
Coordinates Cartesian curvilinear curvilinear curvilinear
Connectivity h-nonconforming conforming conforming h-nonconforming
Adaptive mesh refinement (AMRCallback) ✔️ ✖️ ✖️ ✔️
Domain hypercube mapped hypercube arbitrary arbitrary
Weak form (VolumeIntegralWeakForm) ✔️ ✔️ ✔️ ✔️
Flux differencing (VolumeIntegralFluxDifferencing) ✔️ ✔️ ✔️ ✔️
Shock capturing (VolumeIntegralShockCapturingHG) ✔️ ✖️ ✖️ ✖️

@sloede
Copy link
Member

sloede commented Jul 7, 2021

This looks really great! I think we should put something like this in the docs and maybe also in the ICOSAHOM tutorial? Three and a half suggestions from my side:

  • Use "Feature" as the first column title
  • Add "AMR" row
  • Use English terms for the DG features, eg, weak form, strong form, shock capturing. You can still put the type names in parents below
  • left-align the first column

@ranocha
Copy link
Member Author

ranocha commented Jul 7, 2021

☝️ Like this? (updated the draft above)

@sloede
Copy link
Member

sloede commented Jul 7, 2021

Yes, much better, thank you. I have a few more suggestions now 😉:

  • use ✔️ instead of ✅ to make it stand out more
  • use ✖️ instead of ❌ to make it stand out less
  • for the "conforming" row: maybe use "connectivity" or "topology" as row header? Right now it looks somewhat empty
  • drop the "Hennemann Gassner" from shock capturing - it makes the line wrap and is imho not a vital piece of information
  • move the AMRCallback to the row header (like for the volume integral types)
  • put the type links in (parens) to make it clear that this is a detail and not part of the general description

If you prefer, I can also create a new table below and modify your table above directly.

@ranocha
Copy link
Member Author

ranocha commented Jul 7, 2021

Updated above

@ranocha
Copy link
Member Author

ranocha commented Jul 7, 2021

I think we could also add

global_overview

@ranocha
Copy link
Member Author

ranocha commented Jul 7, 2021

I think we should put something like this in the docs and maybe also in the ICOSAHOM tutorial?

Could you add it to the ICOSAHOM tutorial?

@ranocha ranocha self-assigned this Jul 7, 2021
@jlchan
Copy link
Contributor

jlchan commented Jul 7, 2021

I really like this layout! Would it be OK for me to add simplicial meshes to the table after the PR is merged?

@ranocha
Copy link
Member Author

ranocha commented Jul 7, 2021

Sure, that's the idea 👍

@andrewwinters5000
Copy link
Member

I like this summary table a lot! It is a very nice snapshot of Trixi meshes and features. It also indicates directly where I need to work on the unstructured solver :)

@sloede
Copy link
Member

sloede commented Jul 7, 2021

I like this summary table a lot! It is a very nice snapshot of Trixi meshes and features. It also indicates directly where I need to work on the unstructured solver :)

We should really sit together (maybe after ICOSAHOM) and discuss what a good strategy would be to extend the unstructured solver. I mean, we can just leave it as it is, but in the back of my mind I have the feeling that the unstructured solver and the P4estMesh solver share too many similarities to be two completely separate solvers. Conceptually, I view the P4estMesh as the unstructured mesh plus h-adaptivity, thus from a very high-level point of view, I can't see a reason why there are so many DG-related methods implemented for the 2D P4estMesh (instead of reusing the unstructured stuff). But, as always, there's also a cost associated with unifying everything... Let's set together (maybe also with @efaulhaber) and figure out a good way forward.

@andrewwinters5000
Copy link
Member

andrewwinters5000 commented Jul 7, 2021

Conceptually, I view the P4estMesh as the unstructured mesh plus h-adaptivity,

I agree! Their structure is similar (apart from possible differences in local edge numbering) and the functionality goals are the same; real world, interesting problems in 2D/3D curvilinear coordinates.

Let's set together (maybe also with @efaulhaber) and figure out a good way forward.

Definitely, I am game to concoct a plan for these features moving forward.

EDIT: We should also include @Cczernik in this discussion. He is working on curvilinear shock capturing and already has some nice results for compressible Euler in 2D

@sloede
Copy link
Member

sloede commented Jul 7, 2021

EDIT: We should also include @Cczernik in this discussion. He is working on curvilinear shock capturing and already has some nice results for compressible Euler in 2D

As a side note to that, @efaulhaber also has done some work on 3D shock capturing for the P4estMesh, although I am not sure in what kind of state this is.

@efaulhaber
Copy link
Member

So far, the 3D shock capturing works, but I haven't verified that thoroughly (yet). And my implementation definitely needs to be revised before it's ready to be merged anywhere, it's more of a proof-of-concept yet.

@sloede
Copy link
Member

sloede commented Jul 7, 2021

@ranocha I think you're right: While the table above looks great now directly on GH,
image

it is broken for Documenter.jl:
image

Thus my question is: will this table only be used in Documenter.jl or also in the README that's visible directly on GitHub? In the former case, I think we should just use proper unicode symbols, e.g., ✅ and ❌. That would make the table look like this (at least on my computer):
image

What do you think? I think @ranocha essentially suggested something like this in the first place, but I didn't realize what the issue really was.

@Cczernik
Copy link
Contributor

Cczernik commented Jul 7, 2021

The shock-capturing for the unstructured quad mesh in 2D seems to work fine (FSP, EC, Sedov).

But it‘s still not speed optimized. I will work on that and make a PR next week.

@ranocha
Copy link
Member Author

ranocha commented Jul 8, 2021

Let's continue the discussion of the layout in #703

@ranocha
Copy link
Member Author

ranocha commented Jul 8, 2021

I definitely agree that it would be great to unify these approaches and get shock capturing on curvilinear meshes. Some things that might be considered before we can really see P4estMesh{2} as superior to UnstructuredQuadMesh2D are

By the way, I still think it is useful to have a pure Julia implementation of the unstructured mesh available since there can sometimes be subtle problems when relying on binary dependencies, cf. #665 and trixi-framework/P4est.jl#37

@sloede
Copy link
Member

sloede commented Jul 8, 2021

By the way, I still think it is useful to have a pure Julia implementation of the unstructured mesh available

Absolutely, I wholeheartedly agree. I was thinking/hoping it would be possible to implement it in such a way that all the numerics live in the unstructured solver and that P4estMesh essentially is a smart wrapper around an unstructured mesh. But this might turn out to be too high of a cost in terms of complexity, so we should tread carefully here.

What is imho definitely in the books, though, is to make the P4estMesh accept an UnstructuredMesh in the constructor as a base mesh. This would encapture very nicely that P4estMesh "just" adds adaptivity to the mix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants