Skip to content

Feature: extend leaf face neighbor to non-balanced forests and ghosts [N/N]#1736

Open
holke wants to merge 220 commits intomainfrom
feature-extend_lfn_to_ghosts
Open

Feature: extend leaf face neighbor to non-balanced forests and ghosts [N/N]#1736
holke wants to merge 220 commits intomainfrom
feature-extend_lfn_to_ghosts

Conversation

@holke
Copy link
Collaborator

@holke holke commented Jul 4, 2025

Closes #1737 and closes #1299

Will also close #328

Disclaimer

This PR is not 2k lines as it shows currently.
It is based on many changes that are parts of different PRs but until these are not merged, their lines will show up in the count here.
I listed all the PRs below and also marked the sections that do not need review in the "Files Changed" tab.

Describe your changes here:

This has gotten quite large and will be split up and take some time.
I am putting this up here now to start the review early.

I would like to have a review on this because the algorithm is now working.
The review can focus mostly on the leaf_face_neighbor functionality.
It is still in draft status, because

a) there is still low-level functionality missing to make this work on all element shapes
b) i will split this up into multiple PRs eventually
c) not all tests pass, due to missing low-level functionality

Here is an overview over the exsiting PRs that cover changes from this one.
So a review of this PR can exclude all the listed once below:

Description

This PR extends the leaf face neighbor computation in two ways simultaneously:
a) extend it to ghosts, thus we can compute neighbors of ghosts and neighbors that are ghosts
b) extend it to non-balanced forests. So far a 2:1 condition was required, this is no longer the case.

The main contributions are

  • extending many "low-level" element and element array handling functions to ghosts (Could be own PR)
  • extending the face iteration function to ghosts
  • t8_element_array_find function to search for an element in an array (Should be its own PR)
  • t8_forest_leaf_face_neighbors_ext extension to non-balances and ghosts
  • t8_forest_same_level_leaf_face_neighbor_index to compute the data indices of neighbors if only single neighbor (TODO: this was
    implemented while the lfn function did not work for adaptive forests, need to check whether to extend this function to all forests now) (Should be its own PR)
  • a test for the t8_forest_leaf_face_neighbors_ext and t8_forest_same_level_leaf_face_neighbor_index
  • Common header for unit test adapt callbacks that we can reuse (included in Feature: Ancestor search #1714)
  • A new functionality to compute the "Ancestor face" - given a face of an element and a coarser level where the face is a subface of, compute the face number at the coarser level

Edit: The function was so far only tested for quads and hexes due to the missing face ancesor #1738. A first test with lines does fail - i need to investigate what is going on. (t8_gtest_face_neighbors/forest_face_neighbors.test_face_neighbors/t8_cmesh_new_from_class_Line_sc_MPI_COMM_WORLD_default)
Edit2: It now works for all element shapes :)

What is still open to get this out of draft:

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

holke added 30 commits November 13, 2024 13:27
@holke holke self-assigned this Mar 20, 2026
@holke
Copy link
Collaborator Author

holke commented Mar 20, 2026

Thanks for the comments.
I am currently working on integrating them.

for (ichild = 0; ichild < num_face_children; ichild++) {
/* find the owner */
owner = t8_forest_element_find_owner (forest, neighbor_tree, half_neighbors[ichild], neigh_class);
size_t iowner;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
size_t iowner;

t8_forest_element_owners_at_neigh_face (forest, itree, elem, iface, &owners);
/* Iterate over all owners and if any is not the current process,
* add this element as remote */
for (iowner = 0; iowner < owners.elem_count; iowner++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for (iowner = 0; iowner < owners.elem_count; iowner++) {
for (size_t iowner = 0; iowner < owners.elem_count; iowner++) {

else {
int dummy_neigh_face;
/* This element has maximum level, we only construct its neighbor */
neighbor_tree = t8_forest_element_face_neighbor (forest, itree, elem, half_neighbors[0], neigh_class,
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should wait for #2214 such that we can pass a nullptr here.

connected adaptive space-trees of general element classes in parallel.

Copyright (C) 2015 the developers
Copyright (C) 2024 the developers
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Copyright (C) 2024 the developers
Copyright (C) 2026 the developers

Set the proper return values of the leaf face neighbor computation
in case that no neighbors are found.
*/
static void
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please document this function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

}
}

/* TODO: If the forest has no ghosts, then skip the ghosts
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this still an open ToDo? Please open an issue if so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not a TODO anymore. I remove the comment.

// We add the ghost elements of that tree to our search array.
const t8_element_array_t *ghost_leaves
= t8_forest_ghost_get_tree_leaf_elements (forest, local_neighbor_ghost_treeid);
if (ghost_leaves != nullptr) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (ghost_leaves != nullptr) {
if (ghost_leaves != NULL) {

or use if (ghost_leaves)

@Davknapp Davknapp removed their assignment Mar 23, 2026
holke and others added 9 commits March 24, 2026 12:39
Co-authored-by: David Knapp <david.knapp@dlr.de>
Co-authored-by: David Knapp <david.knapp@dlr.de>
Co-authored-by: David Knapp <david.knapp@dlr.de>
Co-authored-by: David Knapp <david.knapp@dlr.de>
Co-authored-by: David Knapp <david.knapp@dlr.de>
@holke
Copy link
Collaborator Author

holke commented Mar 24, 2026

I am waiting to resolve the merge conflicts until i have included your review comments.
Merging this with main is always annoying and time consuming to get it right, so i'd rather do it once at the end.

@Davknapp
Copy link
Collaborator

I am waiting to resolve the merge conflicts until i have included your review comments. Merging this with main is always annoying and time consuming to get it right, so i'd rather do it once at the end.

Ok, then assign me again as soon as you have addressed my comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

5 participants