Skip to content

Fix PFC on root level elements #2280

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
Niklas997 wants to merge 2 commits into
base: main
Choose a base branch
from
+5 −1
Open

Fix PFC on root level elements #2280

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5293c0
Copy root element instead of returning a pointer to the root element …
Niklas997 Apr 17, 2026
591bc9e
Added comments
Niklas997 Apr 17, 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
6 changes: 5 additions & 1 deletion src/t8_forest/t8_forest_pfc_message.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,11 @@ struct t8_forest_pfc_message

// If we are already the root element, we cannot be part of a split family, so we send any(the root) element and no num_siblings.
if (scheme->element_get_level (eclass, element_closest_to_receiver) == 0) {
parent = element_closest_to_receiver;
// Allocate memory for the parent element.
t8_element_new (scheme, eclass, 1, &parent);
Comment on lines +217 to +218
Copy link
Copy Markdown
Collaborator

@spenke91 spenke91 Apr 20, 2026

Choose a reason for hiding this comment

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

Would you mind moving this part before the if statement (and deleting line 226) to avoid code duplication?

Copy link
Copy Markdown
Collaborator Author

@Niklas997 Niklas997 Apr 21, 2026

Choose a reason for hiding this comment

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

Yes, we can do this. Do you think we need to add an additional test or include this as a test case in the PFC test? Since this root level case is not yet captured in the code coverage, the codecov/patch test in this PR fails or is the quick fix enough for now?

Copy link
Copy Markdown
Collaborator

@spenke91 spenke91 Apr 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I'm sorry, but it would be much cleaner to test it. The easiest (though not the most runtime-efficient) option might be to add the level as a test parameter in test/t8_forest/t8_gtest_partition_for_coarsening.cxx , e.g., like this:

INSTANTIATE_TEST_SUITE_P (t8_gtest_partition_for_coarsening, t8_test_partition_for_coarsening_test,
                          testing::Combine (AllSchemeCollections, AllCmeshsParam, testing::Values(0,2)));

And then of course adjust the std::tuple and read the level from std::get<2> (GetParam ()) instead of hard-coding it to 2...

This way, we would test all cmeshes with level 0 and level 2 (because as you said, running with level 0 causes exatcly this issue). If this gets too expensive, we might also change it later e.g. to using level 1 instead of two for TEST_LEVEL_BASIC. But the important thing for me right now would be to cover this issue you found 👍

Would you mind to / know how to adjust the test case? 🤔

Copy link
Copy Markdown
Collaborator Author

@Niklas997 Niklas997 Apr 22, 2026

Choose a reason for hiding this comment

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

Okay, thank you for the hints to do so. Yes, I will try to adapt the test and add it to this PR.

Copy link
Copy Markdown
Collaborator

@spenke91 spenke91 May 4, 2026

Choose a reason for hiding this comment

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

Thank you! :-)

// Copy the root element to the allocated parent element.
scheme->element_copy (eclass, element_closest_to_receiver, parent);
// Set the number of siblings to zero, since there are none on the root level.
num_siblings = 0;
}
else {
Expand Down
Loading