Feature: Ancestor search by holke · Pull Request #1714 · DLR-AMR/t8code

holke · 2025-06-23T09:21:04Z

Describe your changes here:

Currently waiting for #1640 to merge.
Will add a description afterwards.

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.
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 script/find_all_source_files.scp 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).

…docstring still missing

codecov · 2025-06-23T14:19:36Z

Codecov Report

❌ Patch coverage is 90.90909% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.29%. Comparing base (98a2c07) to head (468ea5a).
⚠️ Report is 2 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1714      +/-   ##
==========================================
+ Coverage   78.25%   78.29%   +0.04%     
==========================================
  Files         114      114              
  Lines       19050    19117      +67     
==========================================
+ Hits        14907    14968      +61     
- Misses       4143     4149       +6

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

Davknapp

A first round of remarks! Thanks for this addition!

src/t8_forest/t8_forest_private.cxx

Co-authored-by: David Knapp <david.knapp@dlr.de>

…ature-ancestor_search

test/t8_forest/t8_gtest_bin_search.cxx

Davknapp · 2026-01-30T08:37:07Z

test/t8_forest/t8_gtest_bin_search.cxx

+Due to search calling get_linear_id with a too large level.
+*/
+
+        const t8_locidx_t search_index = t8_forest_bin_search_lower (leafs, element_id + 1, element_id_at_next_level);


Suggested change

const t8_locidx_t search_index = t8_forest_bin_search_lower (leafs, element_id + 1, element_id_at_next_level);

const t8_locidx_t search_index = t8_forest_bin_search_lower (leafs, element_id_at_next_level, element_id + 1);

…search

Co-authored-by: David Knapp <david.knapp@dlr.de>

holke · 2026-02-24T12:26:33Z

I added the tests for bin_search_upper and lower.

Some doxygen errors show up but they are not related to my added functionality...
I need to finalize a comment in the test.

But, you can finish with the review of the code :)

…ature-ancestor_search

holke · 2026-02-24T14:39:10Z

I added a new test for the ancestor search.
Test coverage should now be good.

Davknapp · 2026-02-27T08:09:15Z

I added a new test for the ancestor search. Test coverage should now be good.

Nice, thanks a lot! Can you fix the doxygen?

Davknapp · 2026-02-27T08:50:27Z

test/t8_forest/t8_gtest_bin_search.cxx


+/** Test the t8_forest_bin_search_upper function.
+ * We iterate through all elements of a forest.
+ * For each element E of level L, we call t8_forest_bin_search_lower on the forest's element array and expect


Suggested change

* For each element E of level L, we call t8_forest_bin_search_lower on the forest's element array and expect

* For each element E of level L, we call t8_forest_bin_search_upper on the forest's element array and expect

Davknapp · 2026-02-27T08:53:30Z

test/t8_forest/t8_gtest_bin_search.cxx

+ * the element to be found.
+ * For each element we then build one case where we search for an element that is not contained but will
+ * match a different element in the element array:
+ *  We compute the linear Id of E at level L-1 (if it is >=0 ) and search for the L-1 element with this id.


Suggested change

* We compute the linear Id of E at level L-1 (if it is >=0 ) and search for the L-1 element with this id.

* We compute the linear Id of E at level L+1 (if it is >=0 ) and search for the L+1 element with this id.

Davknapp · 2026-02-27T08:54:51Z

test/t8_forest/t8_gtest_bin_search.cxx

+ * the element to be found.
+ * For each element we then build one case where we search for an element that is not contained but will
+ * match a different element in the element array:
+ *  We compute the linear Id of E at level L+1 (if not exceeding the maxlevel) and search for the L+1 element with this id.


Suggested change

* We compute the linear Id of E at level L+1 (if not exceeding the maxlevel) and search for the L+1 element with this id.

* We compute the linear Id of E at level L (if not exceeding the maxlevel) and search for the L element with this id.

Davknapp · 2026-02-27T08:57:41Z

test/t8_forest/t8_gtest_bin_search.cxx

 }

-TEST_P (t8_bin_search_tester, bin_search_lower)
+/** Test the t8_forest_bin_search_upper function.


Suggested change

/** Test the t8_forest_bin_search_upper function.

/** Test the t8_forest_bin_search_first_descendant_ancestor function.

Please update the rest of the comment, too

Davknapp · 2026-02-27T09:00:22Z

test/t8_gtest_macros.hxx

 inline auto print_eclass
  = [] (const testing::TestParamInfo<t8_eclass> &info) { return t8_eclass_to_string[info.param]; };

+/** Define a lambda to beautify gtest output for tuples <level, cmesh>.


Suggested change

/** Define a lambda to beautify gtest output for tuples <level, cmesh>.

/** Define a lambda to beautify gtest output for tuples <<scheme, eclass>, level>.

Davknapp · 2026-02-27T09:01:46Z

test/t8_gtest_macros.hxx

+auto pretty_print_eclass_scheme_and_level
+  = [] (const testing::TestParamInfo<std::tuple<std::tuple<int, t8_eclass_t>, int>> &info) {
+      std::string scheme = t8_scheme_to_string[std::get<0> (std::get<0> (info.param))];
+      std::string eclass = t8_eclass_to_string[std::get<1> (std::get<0> (info.param))];


Suggested change

std::string eclass = t8_eclass_to_string[std::get<1> (std::get<0> (info.param))];

std::string eclass = t8_eclass_to_string[std::get<0> (std::get<1> (info.param))];

holke added 12 commits June 6, 2025 15:13

Add a comment to bin_search

3223c63

Implement t8_forest_bin_search_upper

a3422fc

Implement t8_forest_element_is_ancestor

5584c0f

Add t8_forest_bin_search_first_descendant_ancenstor

5d1564a

fix bin_search_upper

ca8989e

fix element_is_ancestor

d50a3d8

document bin_search_lower

b700bd7

document bin_search_upper

b28d548

Add declaration of +t8_forest_bin_search_first_descendant_ancenstor. …

67d6265

…docstring still missing

t8_forest_bin_search_first_descendant_ancenstor fix parameter

fb0cc9e

Documentation

f168141

Merge remote-tracking branch 'origin/main' into feature-ancestor_search

ecd65df

holke marked this pull request as draft June 23, 2025 09:22

typo

511cbef

holke mentioned this pull request Jun 23, 2025

Tests for the bin_search routines #1716

Open

Merge remote-tracking branch 'origin/main' into feature-ancestor_search

8f9b60e

spenke91 marked this pull request as ready for review June 23, 2025 13:51

spenke91 assigned Davknapp Jun 23, 2025

spenke91 requested a review from Davknapp June 23, 2025 13:52

Davknapp requested changes Jun 24, 2025

View reviewed changes

src/t8_forest/t8_forest_private.cxx Show resolved Hide resolved

src/t8_forest/t8_forest_private.cxx Show resolved Hide resolved

Davknapp reviewed Jun 25, 2025

View reviewed changes

src/t8_forest/t8_forest_private.cxx Outdated Show resolved Hide resolved

src/t8_forest/t8_forest_private.cxx Outdated Show resolved Hide resolved

Davknapp assigned holke and unassigned Davknapp Jun 25, 2025

holke and others added 3 commits July 2, 2025 10:10

Change docstring

488a6bb

Co-authored-by: David Knapp <david.knapp@dlr.de>

Add const to parameter

30bbb69

Co-authored-by: David Knapp <david.knapp@dlr.de>

Merge branch 'main' into feature-ancestor_search

fd182a2

holke requested a review from Davknapp July 2, 2025 08:13

holke assigned Davknapp and unassigned holke Jul 2, 2025

Merge remote-tracking branch 'origin/feature-ancestor_search' into fe…

331cc9b

…ature-ancestor_search

holke dismissed Davknapp’s stale review via 331cc9b January 28, 2026 14:52

holke added 2 commits January 28, 2026 16:44

Add first version of bin search lower test

9559150

Continue test, crashes during search

1579ae4

Davknapp reviewed Jan 30, 2026

View reviewed changes

holke added 6 commits February 3, 2026 10:30

Fix parameter usage

b2ec0df

correct function documentation

df66454

Add test for bin_search_upper

17cf8e7

fix upper test

af53448

put commonly used print function in header

8ce9d58

Merge remote-tracking branch 'origin/main' into feature-ancestor_search

a69cde4

holke requested a review from Davknapp February 3, 2026 12:27

holke assigned Davknapp and holke and unassigned holke and Davknapp Feb 3, 2026

holke and others added 7 commits February 3, 2026 13:40

Implemented test for lower and upper bindary serach

425b28e

Move commonly used adapt function to own header

f9149b0

put commonly used pretty print test function in header

d92d6d9

Merge branch 'feature-ancestor_search' into feature-test_upper_lower_…

410083c

…search

Merge remote-tracking branch 'origin/main' into feature-ancestor_search

9c2d6d3

Made all leafs into leaves

648c614

Apply suggestions from code review

4ae1bcc

Co-authored-by: David Knapp <david.knapp@dlr.de>

holke added 3 commits February 24, 2026 13:28

Update include path

aa02574

Merge remote-tracking branch 'origin/feature-ancestor_search' into fe…

c4cf0a2

…ature-ancestor_search

Added test for ancestor search

468ea5a

Davknapp requested changes Feb 27, 2026

View reviewed changes

	const t8_locidx_t search_index = t8_forest_bin_search_lower (leafs, element_id + 1, element_id_at_next_level);
	const t8_locidx_t search_index = t8_forest_bin_search_lower (leafs, element_id_at_next_level, element_id + 1);

	* For each element E of level L, we call t8_forest_bin_search_lower on the forest's element array and expect
	* For each element E of level L, we call t8_forest_bin_search_upper on the forest's element array and expect

	* We compute the linear Id of E at level L-1 (if it is >=0 ) and search for the L-1 element with this id.
	* We compute the linear Id of E at level L+1 (if it is >=0 ) and search for the L+1 element with this id.

	/** Test the t8_forest_bin_search_upper function.
	/** Test the t8_forest_bin_search_first_descendant_ancestor function.

	/** Define a lambda to beautify gtest output for tuples <level, cmesh>.
	/** Define a lambda to beautify gtest output for tuples <<scheme, eclass>, level>.

	std::string eclass = t8_eclass_to_string[std::get<1> (std::get<0> (info.param))];
	std::string eclass = t8_eclass_to_string[std::get<0> (std::get<1> (info.param))];

Conversation

holke commented Jun 23, 2025 • 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 Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Davknapp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holke commented Feb 24, 2026

Uh oh!

holke commented Feb 24, 2026

Uh oh!

Davknapp commented Feb 27, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

holke commented Jun 23, 2025 •

edited

Loading

codecov bot commented Jun 23, 2025 •

edited

Loading