Skip to content

Full support for ghosted/unghosted distributed triangulations#100

Merged
JordiManyer merged 13 commits intomasterfrom
distributed
Mar 5, 2025
Merged

Full support for ghosted/unghosted distributed triangulations#100
JordiManyer merged 13 commits intomasterfrom
distributed

Conversation

@JordiManyer
Copy link
Copy Markdown
Member

@JordiManyer JordiManyer commented Nov 14, 2024

In GridapDistributed, we generally allow the creation of ghosted/unghosted versions of objects. Although generally we require unghosted triangulations for integration, having both is usually required for more complex stuff.

The original version of the distributed code does not allow this. In fact, the code avoids doing cuts on ghosted cells. This was probably done to avoid repeating work, but it limits the potential of the distributed code. One example where the current implementation is not flexible enough is taking Skeleton/Boundary triangulations of a distributed SubFacetTriangulation, where we require access to the cuts on ghost cells in order to properly distinguish between interior and boundary faces.

In terms of performance, I would argue that creating model views to create the cuts and then mapping the cuts back to the original model is a lot more costly than doing the cuts on the ghost cells then masking out the result.
This also results is quite a lot less code, and better one-to-one comparison with what we do in GridapDistributed.

All in all, I propose changes to address all the above.

@JordiManyer JordiManyer marked this pull request as ready for review November 14, 2024 10:48
@zjwegert
Copy link
Copy Markdown
Contributor

@zjwegert
Copy link
Copy Markdown
Contributor

I've added this in #101

@janmodderman
Copy link
Copy Markdown

janmodderman commented Jan 28, 2025

Hi @JordiManyer,

I think that for Triangulation and each $TT in https://github.com/gridap/GridapEmbedded.jl/blob/6850b686cc41cf5a018436377fc35efa63e60ed2/src/Distributed/DistributedDiscretizations.jl#L63C1-L89C4 we should also pass along ; kwargs... as this is required for obtaining the physical boundary that is intersected by the embedded boundary using BoundaryTriangulation(args...; tags=tags) and this is currently not possible. (See: https://github.com/gridap/GridapEmbedded.jl/blob/6850b686cc41cf5a018436377fc35efa63e60ed2/src/Interfaces/EmbeddedFacetDiscretizations.jl#L72C1-L122C1 )

This is added in #104

Additionally, I have some questions, but if this is not the place to ask these, then please let me know and I'll remove them from this post.

In https://github.com/gridap/GridapEmbedded.jl/blob/6850b686cc41cf5a018436377fc35efa63e60ed2/src/Distributed/DistributedDiscretizations.jl#L60C1-L67C4 it is stated in the comment that the default portion is NoGhost() unless the argument in_or_out is provided, but in the function definition slightly below we only assign WithGhost() for the in_or_out::ActiveInOrOut meaning this will not apply for the CutInOrOut argument thus excluding the CUT and PHYSICAL arguments. Is this intentional? If so, then why?

I'm asking because, I have a box domain and an embedded boundary piercing through the top of this domain and thus I'd like to obtain the physical boundary at the top. I noticed that if I run BoundaryTriangulation(WithGhost(),cutgeofacets, PHYSICAL_OUT; tags=["top"]) I obtain this physical boundary correctly, see image, but when I use NoGhost() or do not include the portion I get an error.

image

@janmodderman
Copy link
Copy Markdown

Check in to leave a note on the question I had, it appears that remove_ghost_cells() in https://github.com/gridap/GridapEmbedded.jl/blob/2b7e04f673fd7dd9a7c2ba8ce2756a732cf21a3f/src/Distributed/DistributedDiscretizations.jl#L98C1-L101C4 is only defined for type SubFacetTriangulation, but the type SubFacetBoundaryTriangulation (from https://github.com/gridap/GridapEmbedded.jl/blob/2b7e04f673fd7dd9a7c2ba8ce2756a732cf21a3f/src/Interfaces/EmbeddedFacetDiscretizations.jl#L228C1-L267C4) appears to be missing for both the function as well as from the Distributed.jl file. Adding these, resolves my issue.

@JordiManyer JordiManyer merged commit 6adb1c4 into master Mar 5, 2025
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.

3 participants