Skip to content

Use firedrake-rtree for spatial indexing#4981

Open
leo-collins wants to merge 42 commits intomainfrom
leo/rust-rstar
Open

Use firedrake-rtree for spatial indexing#4981
leo-collins wants to merge 42 commits intomainfrom
leo/rust-rstar

Conversation

@leo-collins
Copy link
Copy Markdown
Contributor

@leo-collins leo-collins commented Mar 20, 2026 •
edited
Loading

Description

Use firedrake-rtree for spatial indexing.

  • This is an order of magnitude faster than libspatialindex for querying points.
  • We can now build a one dimensional spatial index, allowing us to remove a whole load of special-cased code.
  • We can now access the nodes of the tree and manually traverse it. This will be used to build a distributed rtree which will allow us to use sparse communication operations to build the VertexOnlyMesh, instead of the expensive allgathers we use currently.

leo-collins
leo-collins commented Mar 21, 2026
View reviewed changes
Comment thread firedrake/locate.c Outdated
connorjward
connorjward reviewed Mar 24, 2026
View reviewed changes
Comment thread setup.py
connorjward
connorjward reviewed Mar 25, 2026
View reviewed changes
Comment thread setup.py Outdated
@leo-collins leo-collins mentioned this pull request Mar 25, 2026
Merged
@leo-collins leo-collins marked this pull request as ready for review March 26, 2026 18:33
connorjward
connorjward requested changes Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@connorjward connorjward left a comment

Choose a reason for hiding this comment

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

Just tiny things. I think this is great and 99% ready.

Comment thread firedrake/cython/rtree.pyx
Comment thread firedrake/cython/rtreeinc.pxi Outdated
Comment thread firedrake/mesh.py Outdated
leo-collins added 21 commits April 2, 2026 11:06
@leo-collins
use rstar capi for cell location
8a07281
@leo-collins
add logging
85ae5e9
@leo-collins
remove allgather logs
5169948
@leo-collins
update comments
3b96b1e
@leo-collins
fix cached property
281c971
@leo-collins
merge logging
e8da836 
more logging
@leo-collins
more logs
ab58a9b
@leo-collins
more logging
8f7bd74
@leo-collins
adapt to new API
dcc3052
@leo-collins
rebase
1ffbc5d
@leo-collins
spatial index in 1D
94d0165
@leo-collins
partial fix
37a8a3f
@leo-collins
fix test
c7de8cd 
This test depended on the ordering of the candidate cells
returned by the rtree query. Instead we check the missing
coordinates correctly as suggested by the comment
@leo-collins
tidy up cython
e5722a0
@leo-collins
fixes
613c5f1
@leo-collins
renaming and use firedrake_rtree package
d1b9295
@leo-collins
firedrake-rtree fixes
c291d33
@leo-collins
remove unused stuff from rtree cython
dfe8df1
@leo-collins
free ids in error case
ebd96dc
@leo-collins
add firedrake-rtree dependency
470c1be 
add to build-requirements.txt
@leo-collins
remove unnecessary stuff from petschdr
c7eadf4
@leo-collins
fix docstrings
5395ca6 
docstrings

more comments
@leo-collins leo-collins requested a review from connorjward April 16, 2026 10:36
connorjward
connorjward requested changes Apr 16, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@connorjward connorjward left a comment

Choose a reason for hiding this comment

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

Otherwise this looks good to go.

What's the status of firedrake-rtree? At a minimum you need to add some more people as owners of the PyPI package.

Comment thread pyproject.toml Outdated
Comment thread pyproject.toml Outdated
Comment thread requirements-build.txt Outdated
@leo-collins @connorjward
Apply suggestions from code review
d82c3b1 
Co-authored-by: Connor Ward <c.ward20@imperial.ac.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@connorjward connorjward connorjward requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

macOS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@leo-collins @connorjward