Feature: add neighbor subface helper by dutkalex · Pull Request #2216 · DLR-AMR/t8code

dutkalex · 2026-03-06T14:08:01Z

Describe your changes here:
Implements #2215

All these boxes must be checked by the AUTHOR before requesting review:

The PR is small enough to be reviewed easily. If not, consider splitting up the changes in multiple PRs.
The title starts with one of the following prefixes: Documentation:, Bugfix:, Feature:, Improvement: or Other:.
If the PR is related to an issue, make sure to link it.
The author made sure that, as a reviewer, he/she would check all boxes below.

All these boxes must be checked by the REVIEWERS before merging the pull request:

As a reviewer please read through all the code lines and make sure that the code is fully understood, bug free, well-documented and well-structured.

General

The reviewer executed the new code features at least once and checked the results manually.
The code follows the t8code coding guidelines.
New source/header files are properly added to the CMake files.
The code is well documented. In particular, all function declarations, structs/classes and their members have a proper doxygen documentation. Make sure to add a file documentation for each file!
All new algorithms and data structures are sufficiently optimal in terms of memory and runtime (If this should be merged, but there is still potential for optimization, create a new issue).

Tests

The code is covered in an existing or new test case using Google Test.
The code coverage of the project (reported in the CI) should not decrease. If coverage is decreased, make sure that this is reasonable and acceptable.
Valgrind doesn't find any bugs in the new code. This script can be used to check for errors; see also this wiki article.

If the Pull request introduces code that is not covered by the github action (for example coupling with a new library):

Should this use case be added to the github action?
If not, does the specific use case compile and all tests pass (check manually).

Scripts and Wiki

If a new directory with source files is added, it must be covered by the scripts/internal/find_all_source_files.sh to check the indentation of these files.
If this PR introduces a new feature, it must be covered in an example or tutorial and a Wiki article.

License

The author added a BSD statement to doc/ (or already has one).

codecov · 2026-03-06T14:26:56Z

Codecov Report

❌ Patch coverage is 60.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.52%. Comparing base (f878a8e) to head (a7f955d).
⚠️ Report is 539 commits behind head on main.

Files with missing lines	Patch %	Lines
src/t8_forest/t8_forest.cxx	60.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2216      +/-   ##
==========================================
+ Coverage   78.27%   82.52%   +4.24%     
==========================================
  Files         114      115       +1     
  Lines       19101    18561     -540     
==========================================
+ Hits        14952    15318     +366     
+ Misses       4149     3243     -906

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

spenke91

Thank you very much @dutkalex for another great contribution! :-) I am sorry it took us so long to reply...
I mostly have suggestions regarding variable names and code comments (which, I know, not everybody loves as much as I do 🤓 ). Aside from that, I only have a question regarding the hard-coded 4.

spenke91 · 2026-04-29T07:09:21Z

+  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);


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?

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

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.

spenke91 · 2026-04-29T07:12:28Z

+
+  scheme->element_get_children_at_face (neighbor_tree_class, neighbor_leaf, neighbor_face, children.begin (),
+                                        num_children, nullptr);
+


Suggested change

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

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

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

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

spenke91 · 2026-04-29T07:12:45Z

+      result = i_child;
+    }
+  }
+


Suggested change

// Free memory and return sub-face ID.

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

Maybe target_face_id or something like that?

spenke91 · 2026-04-29T07:17:50Z

+  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);
+
+  int const num_children = scheme->element_get_num_face_children (neighbor_tree_class, neighbor_leaf, neighbor_face);


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

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

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 👍

spenke91 · 2026-04-29T07:22:56Z

+  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);


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

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

because we need an integral constant expression here (i.e. a constexpr int-like thing)

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?

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.

spenke91 · 2026-04-29T07:25:23Z

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


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

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

Co-authored-by: spenke91 <thomas.spenke@dlr.de>

Add assertion to ensure the face was found before destruction.

dutkalex · 2026-05-09T22:30:40Z

Thank you very much @dutkalex for another great contribution! :-) I am sorry it took us so long to reply...

No worries @spenke91. Thank you for taking the time, I know you guys have a lot on your plate.

I mostly have suggestions regarding variable names and code comments (which, I know, not everybody loves as much as I do 🤓 ). Aside from that, I only have a question regarding the hard-coded 4.

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 😉. I have either committed your suggestions directly, or answered your comments/questions to get your opinion. Let me know what you think and I'll revise the code accordingly.

spenke91

Thanks a lot for your comments and replies!

spenke91 · 2026-05-19T09:20:32Z

+  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);


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.

spenke91 · 2026-05-19T09:21:45Z

+
+  scheme->element_get_children_at_face (neighbor_tree_class, neighbor_leaf, neighbor_face, children.begin (),
+                                        num_children, nullptr);
+


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

spenke91 · 2026-05-19T09:22:58Z

+
+  scheme->element_get_children_at_face (neighbor_tree_class, neighbor_leaf, neighbor_face, children.begin (),
+                                        num_children, nullptr);
+


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

spenke91 · 2026-05-19T09:51:36Z

+  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);
+
+  int const num_children = scheme->element_get_num_face_children (neighbor_tree_class, neighbor_leaf, neighbor_face);


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 👍

spenke91 · 2026-05-19T09:57:13Z

+      result = i_child;
+    }
+  }
+


Maybe target_face_id or something like that?

spenke91 · 2026-05-19T10:08:05Z

+  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);


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.

dutkalex added 4 commits March 6, 2026 14:38

add basic implementation

8760dcf

indent

7908780

doc

2e89aef

Merge branch 'main' into add-neighbor-subface-helper

fda9f6b

holke self-assigned this Mar 12, 2026

holke added enhancement Enhances already existing code priority:medium Should be solved within half a year workload:low Would take half a day or less labels Mar 12, 2026

holke assigned spenke91 and benegee and unassigned holke Apr 17, 2026

spenke91 self-requested a review April 29, 2026 07:06

spenke91 requested changes Apr 29, 2026

View reviewed changes

spenke91 assigned dutkalex and unassigned spenke91 Apr 29, 2026

spenke91 mentioned this pull request Apr 29, 2026

Improvement: Cover t8_forest_leaf_neighbor_subface in a test case #2295

Open

dutkalex and others added 2 commits May 9, 2026 23:04

clarify documentation

e265561

Co-authored-by: spenke91 <thomas.spenke@dlr.de>

add assertion to check the result validity

beb432b

Add assertion to ensure the face was found before destruction.

dutkalex added 2 commits May 10, 2026 00:59

clarify documentation

5e29523

fix typo

a7f955d

dutkalex removed their assignment May 14, 2026

dutkalex requested a review from spenke91 May 14, 2026 16:53

spenke91 reviewed May 19, 2026

View reviewed changes

spenke91 assigned dutkalex and unassigned benegee May 19, 2026


		scheme->element_get_children_at_face (neighbor_tree_class, neighbor_leaf, neighbor_face, children.begin (),
		num_children, nullptr);


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

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

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

Conversation

dutkalex commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

General

Tests

Scripts and Wiki

License

Uh oh!

codecov Bot commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

spenke91 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dutkalex May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dutkalex May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dutkalex May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dutkalex commented May 9, 2026

Uh oh!

spenke91 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

dutkalex commented Mar 6, 2026 •

edited

Loading

codecov Bot commented Mar 6, 2026 •

edited

Loading

dutkalex May 9, 2026 •

edited

Loading

dutkalex May 9, 2026 •

edited

Loading

dutkalex May 9, 2026 •

edited

Loading