Fix balance#368
Open
hannesbrandt wants to merge 23 commits into
Open
Conversation
In balance neighborhood_owned skips tree boundary quadrants that are critical for balancing because they intersect the insulation layer of quadrants in edge-neighboring trees. Therefore, neighborhood_boundary_owned is introduced as a function that determines always the full tree boundary if the tree is fully owned. This means that in tree_compute_overlap more quadrants than necessary are considered but this ensures correct results (cf. simple3 example drop).
Collaborator
Author
Of course, we could avoid this regression by checking for face, edge and corner tree connections depending on the btype at the relevant places (cf. #101). |
Owner
|
> The described fixes come with a performance regression ranging from 0%-20% according to our tests. This is most likely due to the borders array, which increases in size by the less strict version of `p4est_comm_neighborhood_owned` . The second fix (no longer skipping isolated trees) should only have an impact on rare connectivites with a high percentage of isolated trees.
Of course, we could avoid this regression by checking for face, edge and corner tree connections depending on the btype at the relevant places (cf. #101).
Thanks, @hbrandt and @tim-griesbach. I'd prefer merging this one first, to
harden balance against rare/evil connectivities, and to add the optimization
(checking tree contacts, this time for all codimensions) at a later point.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Following up on issue #238 and #319 . As discussed in #238, balance (and ghost) use face-only tree connections when creating the
bordersarray at some places independent of thebtype. This leads to unbalanced results ofp4est_balance, e.g. for trees that are connected only by edge/corner.In this PR
p4est_comm_neighborhood_ownedis changed such that all quadrants touching the tree boundary are added to the borders array even if there is no neighboring tree and the function is renamed top4est_comm_is_boundary.The face-only tree connections are also used to identify isolated trees. Therefore, in a connectivity of two trees connected by a single edge or corner both would erroneously be identified as isolated and skipped. To address this skipping of isolated trees was removed. Also, this PR adds a new connectivity edge_corner, which is used to test balance in the mentioned corner cases. It contains three trees with one edge and one corner connection between them.
The described fixes come with a performance regression ranging from 0%-20% according to our tests. This is most likely due to the borders array, which increases in size by the less strict version of
p4est_comm_neighborhood_owned. The second fix (no longer skipping isolated trees) should only have an impact on rare connectivites with a high percentage of isolated trees.Proposed changes:
p4est_comm_neighborhood_ownedtop4est_comm_is_boundary.p4est_comm_is_boundaryand switch return values to better fit the new name.@tisaac