Skip to content

Expose _bbox_coordinates of BoundingBoxTree to users - #3600

Merged
garth-wells merged 6 commits into
FEniCS:mainfrom
ordinary-slim:bbox_coordinates
Mar 2, 2025
Merged

Expose _bbox_coordinates of BoundingBoxTree to users#3600
garth-wells merged 6 commits into
FEniCS:mainfrom
ordinary-slim:bbox_coordinates

Conversation

@ordinary-slim

@ordinary-slim ordinary-slim commented Jan 16, 2025

Copy link
Copy Markdown
Contributor

Addressing #3599. Expose _bbox_coordinates of BoundingBoxTree the same way _x of Geometry is exposed. I added a test with point-tree collisions before and after a translational motion.

@ordinary-slim ordinary-slim changed the title Expose bbox_coordinates of BoundingBoxTree to users Expose _bbox_coordinates of BoundingBoxTree to users Jan 17, 2025
- Avoiding copying of bounding boxes
- Early break in point_in_bbox
@ordinary-slim

Copy link
Copy Markdown
Contributor Author

I've been profiling compute_collisions with points and I found some opportunities for speed-ups:

  • Early break in point_in_bbox. Note that &= doesn't short-circuit and always evaluates the RHS so it's better to have a &= b && c in the same line rather than have it in two separate lines.
  • Avoid calling BoundingBoxTree.get_bbox as it spends a good deal of time in copy_n. I wrote a small workaround as a lambda function in compute_collisions with points but I think it would be cleaner if get_bbox returned a span of const rather than a new copy.

In this snippet of code

from dolfinx import mesh, geometry
import numpy as np
from mpi4py import MPI
from line_profiler import LineProfiler

comm = MPI.COMM_WORLD
rank = comm.Get_rank()

def main():
    tdim = 3
    num_elems_side = 20
    domain = mesh.create_unit_cube(MPI.COMM_WORLD,
                                   num_elems_side, num_elems_side, num_elems_side,
                                   mesh.CellType.tetrahedron, dtype=np.float64)
    bbtree = geometry.bb_tree(domain, tdim, padding=0.0)
    rng = np.random.default_rng(0)
    num_points = 200000
    points = rng.random((num_points, 3))
    collisions = geometry.compute_collisions_points(bbtree, points)

if __name__=="__main__":
    lp = LineProfiler()
    lp_wrapper = lp(main)
    lp_wrapper()
    with open(f"profiling_rank{rank}.txt", 'w') as pf:
        lp.print_stats(stream=pf)

the line collisions = geometry.compute_collisions_points(bbtree, points) takes 2.4 seconds before these changes and 1.95 seconds after these changes.

@jhale

jhale commented Feb 13, 2025

Copy link
Copy Markdown
Member

Could you split these changes into two PRs? The algorithmic changes need a different type of review.

Comment thread cpp/dolfinx/geometry/utils.h Outdated
std::deque<std::int32_t> stack;
std::int32_t next = tree.num_bboxes() - 1;
std::span<const T> bbox_coordinates = tree.bbox_coordinates();
auto view_bbox = [&bbox_coordinates](std::size_t node)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this is needed - use subspan.

@ordinary-slim

Copy link
Copy Markdown
Contributor Author

I'll make the changes as soon as I have time, apologies for the delay.
Just leaving a note here that applying a constant shift does work as intented but rotations or reflections do not work since bboxes are defined by their lower left and upper right coordinates.

@garth-wells garth-wells left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice work.

Resolves #3599.

@garth-wells
garth-wells enabled auto-merge March 2, 2025 17:07
@garth-wells garth-wells added the enhancement New feature or request label Mar 2, 2025
@garth-wells garth-wells added this to the 0.10.0 milestone Mar 2, 2025
@garth-wells
garth-wells added this pull request to the merge queue Mar 2, 2025
Merged via the queue into FEniCS:main with commit 4392b7e Mar 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants