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

Add automatic padding for the FullGridCellList #90

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

efaulhaber
Copy link
Member

@efaulhaber efaulhaber commented Dec 23, 2024

Also adds error messages and closes #65, #80.

@efaulhaber efaulhaber added the enhancement New feature or request label Dec 23, 2024
@efaulhaber efaulhaber requested review from LasNikas and svchb December 23, 2024 11:23
Copy link

codecov bot commented Dec 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.38%. Comparing base (1e25058) to head (c2a4640).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
+ Coverage   88.15%   88.38%   +0.23%     
==========================================
  Files          15       15              
  Lines         498      508      +10     
==========================================
+ Hits          439      449      +10     
  Misses         59       59              
Flag Coverage Δ
unit 88.38% <100.00%> (+0.23%) ⬆️

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.

@efaulhaber efaulhaber self-assigned this Dec 24, 2024
src/nhs_grid.jl Outdated
Comment on lines 209 to 210
check_domain_bounds(neighborhood_search.cell_list, y,
search_radius(neighborhood_search))
Copy link
Collaborator

Choose a reason for hiding this comment

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

In terms of performance, does it make sense to switch off this check for the update?

Copy link
Member Author

@efaulhaber efaulhaber Dec 30, 2024

Choose a reason for hiding this comment

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

You're right. It's slow.

Two options:

  1. Make this parallel like so:
function check_domain_bounds(cell_list::FullGridCellList, y, search_radius)
    valid_ = [true]
    backend = KernelAbstractions.get_backend(y)
    valid = Adapt.adapt(backend, valid_)

    @threaded y for point in 1:size(y, 2)
        coords = extract_svector(y, Val(3), point)
        if any(isnan, coords)
            valid[1] = false
        end

        if any(coords .- search_radius .< cell_list.min_corner) || any(coords .+ search_radius .> cell_list.max_corner)
            valid[1] = false
        end
    end
    if !Vector(valid)[1]
        error("particle coordinates are outside the domain bounds of the cell list")
    end
end

The alternating update benchmark yielded:
7.136 ms with Polyester.jl without this check
8.497 ms with Polyester.jl with the check
20.356 ms with CUDA.jl without the check
27.648 ms with CUDA.jl with the check

  1. Add a check inside the update loop/kernel. This is what is currently implemented in this PR. This approach does not have a measurable overhead, but has the disadvantage that errors are thrown inside a multithreaded loop. With Polyester.jl, threads are just stopped on errors, but no error message is displayed (Threads fail silently on error JuliaSIMD/Polyester.jl#153):
julia> x = zeros(Int, 5, 10)
5×10 Matrix{Int64}:
 0  0  0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0  0  0

julia> @batch for j in axes(x, 2)
           for i in 1:5
               if i > 3 && j in (2, 4, 9)
                   error("some error")
               end
               x[i, j] = 1
           end
       end

julia> x
5×10 Matrix{Int64}:
 1  1  1  1  1  1  1  1  1  1
 1  1  1  1  1  1  1  1  1  1
 1  1  1  1  1  1  1  1  1  1
 1  0  1  0  1  1  1  1  0  1
 1  0  1  0  1  1  1  1  0  1

So in this case, particles outside the domain bounds will cause unpredictable behavior instead of crashing the simulation with an error, as we would expect.

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.

FullGridCellList can't handle NaN values and throws unspecific errors
2 participants