Skip to content

Add option to allow/disallow extrapolation in PointCollisionData. - #4453

Merged
garth-wells merged 11 commits into
mainfrom
dokken/allow-extrapolation
Sep 3, 2026
Merged

Add option to allow/disallow extrapolation in PointCollisionData.#4453
garth-wells merged 11 commits into
mainfrom
dokken/allow-extrapolation

Conversation

@jorgensd

@jorgensd jorgensd commented Aug 28, 2026

Copy link
Copy Markdown
Member

Addresses #4450.

Makes it optional to perform extrapolation within cell bounding box for closest point detection (and in turn non-matching interpolation).

Default behavior of allowing extrapolation is kept to avoid breaking user code.

C++ changes have been co-authored with Claude. Test is based on code from @MathPhy42. I have reviewed the contribution.

Other fixes

  • Buffer-hoisting: In two places in determine_point_ownership there were arrays containing local coordinate_dofs geometry data being made in hot (and very hot) loops. Resolved by pre-allocating a buffer and adding scratch memory to compute_first_colliding_cell.
  • Improved documentation: More explicit documentation as requested by reviewers
  • Subtle bug fix a dist>0 is replaced by dist>=0. Very unlikely to trigger with GJK, as we rarely get machine 0, but better than not having it.

@jorgensd
jorgensd requested a review from jhale August 28, 2026 12:45
Comment thread cpp/dolfinx/geometry/utils.h Outdated
Comment thread python/dolfinx/fem/utils.py Outdated

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

It would be good to run an LLM over the docstrings for the affected functions. They can be made easier to read.

Comment thread cpp/dolfinx/fem/interpolate.h Outdated
jorgensd and others added 4 commits August 31, 2026 07:12
Remove reallocation in hotloop.
Rename extrapolate to find_closest_cell withing determine point ownership (name kept in create_interpolation data).
@jorgensd

jorgensd commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

Further fixes made by Claude and me:

  • Sentinel bug: current_dist > 0current_dist >= 0 in the closest-cell aggregation loop. -1 is the "not computed" sentinel; a genuinely closest candidate can legitimately have squared distance 0 (point exactly on a cell boundary), and the old check rejected that valid answer the same way it rejected the sentinel.
  • Buffer hoisting: compute_first_colliding_cell now takes a coordinate_dofs scratch-buffer span (sized to fit, oversized buffers explicitly allowed for future mixed-topology support) instead of allocating internally. determine_point_ownership hoists one buffer before its point loop and reuses it for both the initial collision loop and the closest-cell fallback loop (previously only the fallback loop had been hoisted).
  • Added a note documenting that find_closest_cell must be uniform across all ranks of mesh.comm(), since it gates collective MPI calls.
  • Renamed send_extrapolate/dest_extrapolate → send_needs_closest_cell/dest_needs_closest_cell for consistency with the renamed public parameter.
    Added missing direct includes: , , , .
  • Replaced auto with explicit types on plain std::int32_t/T values (loop variables over collisions.links(...), and current_dist/d in the aggregation loop), per AGENTS.md.

@jorgensd
jorgensd requested review from garth-wells and jhale August 31, 2026 09:01
@garth-wells
garth-wells added this pull request to the merge queue Sep 3, 2026
Merged via the queue into main with commit 700e977 Sep 3, 2026
22 checks passed
@garth-wells
garth-wells deleted the dokken/allow-extrapolation branch September 3, 2026 09:48
shimwell added a commit to shimwell/dolfinx that referenced this pull request Sep 3, 2026
Picks up FEniCS#4475, which drops the cython<3.3 pin, and FEniCS#4453.

Co-authored-by: jon-proximafusion <174589458+jon-proximafusion@users.noreply.github.com>
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.

3 participants