Skip to content

Feature: add neighbor subface helper #2216

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.

Sign up for GitHub

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

Open
dutkalex wants to merge 8 commits into
base: main
Choose a base branch
from
+53 −0
Open

Feature: add neighbor subface helper #2216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8760dcf
add basic implementation
dutkalex Mar 6, 2026
7908780
indent
dutkalex Mar 6, 2026
2e89aef
doc
dutkalex Mar 6, 2026
fda9f6b
Merge branch 'main' into add-neighbor-subface-helper
dutkalex Mar 6, 2026
e265561
clarify documentation
dutkalex May 9, 2026
beb432b
add assertion to check the result validity
dutkalex May 9, 2026
5e29523
clarify documentation
dutkalex May 9, 2026
a7f955d
fix typo
dutkalex May 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions src/t8_forest/t8_forest.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1885,6 +1885,39 @@ t8_forest_leaf_face_neighbors (t8_forest_t forest, t8_locidx_t ltreeid, const t8
pelement_indices, pneigh_eclass, forest_is_balanced, NULL, NULL);
}

int
t8_forest_leaf_neighbor_subface (t8_forest_t forest, t8_locidx_t ltreeid, const t8_element_t *leaf, int face,
t8_eclass_t neighbor_tree_class, const t8_element_t *neighbor_leaf, int neighbor_face)
{
t8_scheme const *scheme = t8_forest_get_scheme (forest);

t8_element_t *target = nullptr;
scheme->element_new (neighbor_tree_class, 1, &target);

int dummy; // Can't pass a nullptr to t8_forest_element_face_neighbor below (see #2214)
t8_forest_element_face_neighbor (forest, ltreeid, leaf, target, neighbor_tree_class, face, &dummy);
Comment on lines +1894 to +1898
Copy link
Copy Markdown
Collaborator

@spenke91 spenke91 Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can find a more expressive name than target? e.g.

Suggested change
t8_element_t *target = nullptr;
scheme->element_new (neighbor_tree_class, 1, &target);
int dummy; // Can't pass a nullptr to t8_forest_element_face_neighbor below (see #2214)
t8_forest_element_face_neighbor (forest, ltreeid, leaf, target, neighbor_tree_class, face, &dummy);
// Determine the (virtual) face neighbor
// Note: For this function to work properly, this face neighbor has to be a direct child of neighbor_leaf
t8_element_t *virtual_face_neighbor= nullptr;
scheme->element_new (neighbor_tree_class, 1, &virtual_face_neighbor);
int dummy; // Can't pass a nullptr to t8_forest_element_face_neighbor below (see #2214)
t8_forest_element_face_neighbor (forest, ltreeid, leaf, virtual_face_neighbor, neighbor_tree_class, face, &dummy);

Or maybe we can even find something better? sub_neighbor?

Copy link
Copy Markdown
Contributor Author

@dutkalex dutkalex May 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"There are only two hard things in computer science: cache invalidation and naming things." 😉

I see your point, and I guess since I could not come up with a better name at the time and went for the rather undescriptive but concise option. I feel like virtual_face_neighbor is more descriptive than sub_neighbor, but it looses the "it's what we're looking for" idea. It seems hard to find a (reasonably sized) name which encodes unambiguously all this information. Maybe this means a small comment is necessary to clarify exactly what this is... What do you think?


int const num_children = scheme->element_get_num_face_children (neighbor_tree_class, neighbor_leaf, neighbor_face);
Copy link
Copy Markdown
Collaborator

@spenke91 spenke91 Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
int const num_children = scheme->element_get_num_face_children (neighbor_tree_class, neighbor_leaf, neighbor_face);
// Get the number of the neighbor's children touching the neighbor_face or, equivalenlty, the face's number of children.
int const num_neighbor_face_children = scheme->element_get_num_face_children (neighbor_tree_class, neighbor_leaf, neighbor_face);

Copy link
Copy Markdown
Contributor Author

@dutkalex dutkalex May 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see why you'd want the variable name to be a little bit more explicit, but isn't the comment just paraphrasing what element_get_num_face_children's documentation already says?
I tend to think that comments should be used to explain why something is done a certain way, but not what the code does (that should be clear by reading the code, and if you think it's not the case maybe I should change the code then).


std::array<t8_element_t *, 4> children; // assumes a 2:1 balanced forest
scheme->element_new (neighbor_tree_class, 4, children.begin ());

scheme->element_get_children_at_face (neighbor_tree_class, neighbor_leaf, neighbor_face, children.begin (),
num_children, nullptr);
Comment on lines +1902 to +1906
Copy link
Copy Markdown
Collaborator

@spenke91 spenke91 Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may well be that I missed something here, but shouldn't we use num_face_children rather than 4?

Suggested change
std::array<t8_element_t *, 4> children; // assumes a 2:1 balanced forest
scheme->element_new (neighbor_tree_class, 4, children.begin ());
scheme->element_get_children_at_face (neighbor_tree_class, neighbor_leaf, neighbor_face, children.begin (),
num_children, nullptr);
// Create an array containing the neighbor's children at neighbor_face.
std::array<t8_element_t *, num_neighbor_face_children> neighbor_children_at_face;
scheme->element_new (neighbor_tree_class, num_neighbor_face_children, neighbor_children_at_face.begin ());
scheme->element_get_children_at_face (neighbor_tree_class, neighbor_leaf, neighbor_face, neighbor_children_at_face.begin (), num_neighbor_face_children, nullptr);

Copy link
Copy Markdown
Contributor Author

@dutkalex dutkalex May 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two reasons for why we can't use num_face_children/num_neighbor_face_children here:

  1. because we need an integral constant expression here (i.e. a constexpr int-like thing)
  2. because what we need here is the upper-bound of the number of face neighbors (this happens to be 4 for a 2:1 balanced interface) so that this piece of code works for any element type

Maybe there is a properly named global constant which could be used instead of this magic number?


Copy link
Copy Markdown
Collaborator

@spenke91 spenke91 Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Iterate over the neighbor's children and compare with virtual face neighbor to find the sub-face ID.

Copy link
Copy Markdown
Contributor Author

@dutkalex dutkalex May 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is meant to be a comment. However, after re-reading with fresh eyes this piece of code, I think this is just a std::find_if and that refactoring the code to use this is the proper way to make the intent explicit here

int result = -1;
for (int i_child = 0; i_child < num_children; ++i_child) {
if (scheme->element_compare (neighbor_tree_class, target, children[i_child]) == 0) {
Comment on lines +1909 to +1910
Copy link
Copy Markdown
Collaborator

@spenke91 spenke91 Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (int i_child = 0; i_child < num_children; ++i_child) {
if (scheme->element_compare (neighbor_tree_class, target, children[i_child]) == 0) {
for (int i_child = 0; i_child < num_neighbor_face_children; ++i_child) {
if (scheme->element_compare (neighbor_tree_class, virtual_face_neighbor, neighbor_children_at_face[i_child]) == 0) {

Copy link
Copy Markdown
Contributor Author

@dutkalex dutkalex May 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make the adequate changes here when the previous comments are settled.

result = i_child;
}
}
T8_ASSERT(result != -1); // make sure the face was found

Copy link
Copy Markdown
Collaborator

@spenke91 spenke91 Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Free memory and return sub-face ID.

Copy link
Copy Markdown
Contributor Author

@dutkalex dutkalex May 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the "free memory" part is quite self evident here, but the "return sub-face ID" part is indeed not clear with the current state of the code. However maybe this just means that result is a bad name ? 😅

Comment thread
dutkalex marked this conversation as resolved.
scheme->element_destroy (neighbor_tree_class, 4, children.begin ());
scheme->element_destroy (neighbor_tree_class, 1, &target);
return result;
}

void
t8_forest_print_all_leaf_neighbors (t8_forest_t forest)
{
Expand Down
20 changes: 20 additions & 0 deletions src/t8_forest/t8_forest_general.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,6 +645,26 @@ t8_forest_leaf_face_neighbors_ext (t8_forest_t forest, t8_locidx_t ltreeid, cons
t8_locidx_t **pelement_indices, t8_eclass_t *pneigh_eclass, int forest_is_balanced,
t8_gloidx_t *gneigh_tree, int *orientation);

/** Compute the subface index for a coarser neighbor
* \param [in] forest The forest. Must be committed.
* \param [in] ltreeid A local tree id.
* \param [in] leaf A leaf in \a ltreeid.
* \param [in] face The face index of \a leaf to consider.
* \param [in] neighbor_tree_eclass The eclass of the neighbor element.
* \param [in] neighbor_leaf The leaf of \a forest on the other side of the face of index \a face of element \a leaf.
* \param [in] neighbor_face The face index of \a neighbor_leaf (i.e. the dual face of \a face).
* \returns The index of the subface of \a neighbor_face which corresponds to \a face.
* \pre \a leaf and \a neighbor_leaf must be a face neighbors. The common face must correspond to \a face for \a leaf
* and \a neighbor_face for \a neighbor_leaf respectively. \a neighbor_leaf must be one level coarser than \a leaf.
* Otherwise the behavior is undefined.
* \note This function is designed to be called after \ref t8_forest_leaf_face_neighbors_ext to complement its output.
* It is primarily intended for balanced forests, but can be used on any committed forest as long as the preconditions
* hold (i.e. the forest must be ''locally balanced'').
*/
Comment thread
dutkalex marked this conversation as resolved.
int
t8_forest_leaf_neighbor_subface (t8_forest_t forest, t8_locidx_t ltreeid, const t8_element_t *leaf, int face,
t8_eclass_t neighbor_tree_class, const t8_element_t *neighbor_leaf, int neighbor_face);

/** Exchange ghost information of user defined element data.
* \param [in] forest The forest. Must be committed.
* \param [in] element_data An array of length num_local_elements + num_ghosts
Expand Down
Loading