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?

Copy link
Copy Markdown
Collaborator

@spenke91 spenke91 May 19, 2026

Choose a reason for hiding this comment

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

Love the quote!😄

I think a comment is a good idea anyway (as I said, I will basically never say "no" to code comments 🙈 )
We all do not love long variable names, but what's the harm of using maybe target_virtual_neighbor or even target_virtual_face_neighbor -- after all, this isn't some old Fortran code with 72 characters per line 😉
With a comment, however, I also don't mind sticking to target if that's what you prefer.


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).

Copy link
Copy Markdown
Collaborator

@spenke91 spenke91 May 19, 2026

Choose a reason for hiding this comment

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

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...)

I guess I have a fundamentally different philosophy here, but that is definitely not a problem at all! :-) Because it is in fact just my personal preference and not part of any t8code coding guidelines. So if you feel like some of the comments I suggested are too much, don't worry and feel free not to add them 👍

In case you are interested, my philosphy is more that reading the doxygen header of a function along with the inline code comments should already give an idea about what the code does -- and how it does it, meaning the structure of the algorithm. For example, the line code comments I suggested alone would read

  // Determine the (virtual) face neighbor
  // Note: For this function to work properly, this face neighbor has to be a direct child of neighbor_leaf

  // Get number of neighbor's children

  // Allocate memory

  // Iterate over the neighbor's subfaces and compare with target to find the sub-face ID.

  // Free memory and return sub-face ID.

which, I'd argue, already gives you a rough but useful outline of the function without reading one single line of code.
The motivation behind this is that I am convinced even a proficient software developer can still read English faster than C++ 😉

(And as for the added work of writing the comments, I am certain the time is neglible compared to the time saved for future readers -- after all, while writing the code we know what we're doing anyway!)

So regarding your quote here

I get it, I often say that getting the compiler to understand the code is the easy part and that the real challenge it to write code that humans can comprehend 😉

this would be my answer, that is, paraphrasing the code in a language made for humans to understand, not compilers 😉
But as I said, I know not everybody would agree, which is fine 👍


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 May 19, 2026

Choose a reason for hiding this comment

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

Whoops, the integral constant expression point is in fact quite obvious here, sorry for missing that.

How about defining a new constant to t8_eclass.h

/** The maximal number of children a face may have.*/
#define T8_ECLASS_MAX_FACE_CHILDREN 4

or

/** The maximal number of children a 2D element may have.*/
#define T8_ECLASS_MAX_CHILDREN_2D 4

and using it here? Then it should also be clear where the number 4 comes from.


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

Copy link
Copy Markdown
Collaborator

@spenke91 spenke91 May 19, 2026

Choose a reason for hiding this comment

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

Ah, of course this should be a comment not C++ code 😬

Copy link
Copy Markdown
Collaborator

@spenke91 spenke91 May 19, 2026

Choose a reason for hiding this comment

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

Good idea, std::find_if would make it nicer and clearer 👍

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 ? 😅

Copy link
Copy Markdown
Collaborator

@spenke91 spenke91 May 19, 2026

Choose a reason for hiding this comment

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

Maybe target_face_id or something like that?

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