Spatial Hashing

[RubberLanding]

Implement spatial hashing data structure for neighborhood search.

Depends on #86.

[RubberLanding]

@efaulhaber Please take a look at the current state of the spatial hashing implementation.

[RubberLanding]

The problem is that the current code does not set the collision flag correctly for empty cells without any points.
We only set the collision flag when we call initialize!() which in turn calls push_cell!() for all points. If there is an empty cell A that has a hash collision with a non-empty cell B, the collision flag is not set for this hash collision since we did not add a point from A where we would notice the collision.

function push_cell!(cell_list::SpatialHashingCellList, cell)
	(; cell_coords, cell_collision, list_size, NDIMS) = cell_list
	key = spatial_hash(cell, list_size)
	cell_coord = cell_coords[key]
	if cell_coord == ntuple(_ -> typemin(Int), NDIMS)
		cell_coords[key] = cell
	elseif cell_coord != cell
		cell_collision[key] = true
	end
end

The issue occurs when using _foreach_neighbor(), where we iterate over all neighboring cells, and all points in these cells. The points to iterate over are obtained by computing the hash of a neighboring cell X and collecting the points stored at this hash location (via points_in_cell(index) -> cell_list[index]). If the collision flag at this hash location is set, we compute the cell for each point and check if its the same as the current cell X.
If we iterate over an empty cell A, we collect all points stored at A's hash location (i.e. since A and B have a hash collision, all the points that are actually in cell B). But since the collision flag is not set, we do not check whether a point actually is in A.

function check_collision(neighbor_cell_::Tuple, neighbor_coords,
                         cell_list::SpatialHashingCellList, nhs)
    (; list_size, cell_collision) = cell_list

    if cell_collision[spatial_hash(neighbor_cell_, list_size)]
        # Check if `neighbor_coords` are in the cell `neighbor_cell_`
        cell_coords_ = cell_coords(neighbor_coords, nhs)
        return neighbor_cell_ != cell_coords_
    end
    return false
end

@inline function __foreach_neighbor(f, system_coords, neighbor_system_coords,
                                    neighborhood_search::GridNeighborhoodSearch,
                                    point, point_coords, search_radius)
    (; cell_list, periodic_box) = neighborhood_search
    cell = cell_coords(point_coords, neighborhood_search)

    for neighbor_cell_ in neighboring_cells(cell, neighborhood_search)
        neighbor_cell = Tuple(neighbor_cell_)
        neighbors = points_in_cell(neighbor_cell, neighborhood_search)

        for neighbor_ in eachindex(neighbors)
            neighbor = @inbounds neighbors[neighbor_]

            # Making the following `@inbounds` yields a ~2% speedup on an NVIDIA H100.
            # But we don't know if `neighbor` (extracted from the cell list) is in bounds.
            neighbor_coords = extract_svector(neighbor_system_coords,
                                              Val(ndims(neighborhood_search)), neighbor)

            pos_diff = point_coords - neighbor_coords
            distance2 = dot(pos_diff, pos_diff)

            pos_diff, distance2 = compute_periodic_distance(pos_diff, distance2,
                                                            search_radius,
                                                            periodic_box)

            if distance2 <= search_radius^2
                distance = sqrt(distance2)

                # Check if `neighbor_coords` are in the cell `neighbor_cell_`.
                # For the `SpatialHashingCellList`, this might not be the case
                # if we have a collision.
                if check_collision(neighbor_cell_, neighbor_coords, cell_list,
                                   neighborhood_search)
                    continue
                end

                # Inline to avoid loss of performance
                # compared to not using `foreach_point_neighbor`.
                @inline f(point, neighbor, pos_diff, distance)
            end
        end
    end
end

[efaulhaber]

Would this work?

function check_collision(neighbor_cell::Tuple, neighbor_coords,
                         cell_list::SpatialHashingCellList, nhs)
    (; list_size, cell_collision) = cell_list

    hash = spatial_hash(neighbor_cell_, list_size)
    if cell_collision[hash]
        # Points from multiple cells are in this list.
        # Check if `neighbor_coords` are in the cell `neighbor_cell`.
        return neighbor_cell != cell_coords(neighbor_coords, nhs)
    elseif cell_coords[hash] != neighbor_cell
        # `cell_collision` is false for this list, meaning only points from one cell are in the list.
        # However, the cell of this list is not our `neighbor_cell`, so `neighbor_coords`
        # are not in `neighbor_cell`.
        return true
    end
    return false
end

[efaulhaber]

@RubberLanding #86 is merged now. You can go ahead and prepare this PR for review.

[efaulhaber]

Just a few minor remarks. Otherwise, this is ready to merge from my side. @LasNikas @svchb please have a look as well.

[Copilot]

Pull Request Overview

This pull request implements a spatial hashing data structure for neighborhood search and removes some dependencies from the test configuration to streamline the project setup.

• Removed dependencies and compatibility requirements from test/Project.toml
• Cleans up the test configuration to support the new spatial hashing implementation

Files not reviewed (6)

• src/PointNeighbors.jl: Language not supported
• src/cell_lists/cell_lists.jl: Language not supported
• src/cell_lists/spatial_hashing.jl: Language not supported
• src/nhs_grid.jl: Language not supported
• test/cell_lists/spatial_hashing.jl: Language not supported
• test/neighborhood_search.jl: Language not supported

Comments suppressed due to low confidence (1)

test/Project.toml:1

• Ensure that removing these dependencies is intentional and that no tests rely on them for the spatial hashing functionality.

-   -[deps]

[codecov]

Codecov Report

Attention: Patch coverage is 79.10448% with 14 lines in your changes missing coverage. Please review.

Project coverage is 87.94%. Comparing base (d4944f5) to head (7ce370c).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/cell_lists/spatial_hashing.jl	79.16%	10 Missing ⚠️
src/nhs_grid.jl	88.23%	2 Missing ⚠️
test/cell_lists/spatial_hashing.jl	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #101      +/-   ##
==========================================
- Coverage   89.02%   87.94%   -1.09%     
==========================================
  Files          13       15       +2     
  Lines         556      622      +66     
==========================================
+ Hits          495      547      +52     
- Misses         61       75      +14     

Flag	Coverage Δ
unit	87.94% <79.10%> (-1.09%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[efaulhaber]

LGTM, but the format check is failing.

[svchb]

@efaulhaber If you are starting to add references you should also add a bib file and setup stub docs.

[svchb]

@RubberLanding For the formatting test to not fail you need to use JuliaFormatter.jl and apply it e.g.

using JuliaFormatter
format(".")

[efaulhaber]

@efaulhaber If you are starting to add references you should also add a bib file and setup stub docs.

We have references already. But yes, at some point we should do that.