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

Full support for ghosted/unghosted distributed triangulations #100

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

JordiManyer
Copy link
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
Contributor

I've added this in #101

@janmodderman
Copy link

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

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.

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