Skip to content
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.

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

Fix bounding box leaf collision check #2850

Merged
merged 9 commits into from
Nov 3, 2023

Conversation

jorgensd
Copy link
Sponsor Member

@jorgensd jorgensd commented Nov 2, 2023

Reported by @nate-sime
The following MWE produces the wrong collision for the global_bb_tree

import numpy as np
import dolfinx
from mpi4py import MPI

mesh = dolfinx.mesh.create_unit_square(MPI.COMM_WORLD, 10, 10)

xp = np.array([[100.0, 100.0, 0.0]], dtype=np.double)

tree = dolfinx.geometry.bb_tree(mesh, mesh.topology.dim, padding=1e-4)
global_tree = tree.create_global_tree(mesh.comm)


def pprint(*msg):
    print(f"[{mesh.comm.rank}]: " + " ".join(map(str, msg)), flush=True)


pprint(dolfinx.common.git_commit_hash)
pprint(dolfinx.geometry.compute_collisions_points(tree, xp))
pprint(dolfinx.geometry.compute_collisions_points(global_tree, xp))

giving the following in serial:

[0]: f488f4ecc687d0065315bc9a74939634b18e11ff
[0]: <AdjacencyList> with 1 nodes
  0: []

[0]: <AdjacencyList> with 1 nodes
  0: [0 ]

and the correct result in parallel

[0]: f488f4ecc687d0065315bc9a74939634b18e11ff
[0]: <AdjacencyList> with 1 nodes
  0: []

[0]: <AdjacencyList> with 1 nodes
  0: []

[1]: f488f4ecc687d0065315bc9a74939634b18e11ff
[1]: <AdjacencyList> with 1 nodes
  0: []

[1]: <AdjacencyList> with 1 nodes
  0: []

This is due to a bug in _compute_collisions_point where we don't check if the leaf node actually collides with the point.
@massimiliano-leoni could you see if this impacts your mesh interpolation (in serial)? As the global_bb_tree is used it would add any interpolation point from the other mesh to the process, even if it doesn't collide.

Copy link
Member

@garth-wells garth-wells left a comment

Choose a reason for hiding this comment

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

Could the PR have a more descriptive title?

And could a test be added?

cpp/dolfinx/geometry/utils.h Outdated Show resolved Hide resolved
cpp/dolfinx/geometry/utils.h Outdated Show resolved Hide resolved
cpp/dolfinx/geometry/utils.h Outdated Show resolved Hide resolved
@jorgensd jorgensd changed the title Check leaf collision Fix bounding box leaf collision check Nov 2, 2023
cpp/dolfinx/geometry/utils.h Outdated Show resolved Hide resolved
cpp/dolfinx/geometry/utils.h Outdated Show resolved Hide resolved
@jorgensd
Copy link
Sponsor Member Author

jorgensd commented Nov 3, 2023

@garth-wells Test added + minor simplification. Feel free to merge this once the CI passes.

@garth-wells garth-wells added this pull request to the merge queue Nov 3, 2023
Merged via the queue into main with commit 8bfea1a Nov 3, 2023
20 checks passed
@garth-wells garth-wells deleted the dokken/fix-bb-point-collision branch November 3, 2023 08:58
jhale pushed a commit that referenced this pull request Nov 6, 2023
* Check leaf collision

* Reviewer comments

* Fix the fix

* Apply suggestions from code review

* Make logic clearer

* Readability fix

* Remove superfluous variable

* Add test

---------

Co-authored-by: Garth N. Wells <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants