-
Notifications
You must be signed in to change notification settings - Fork 119
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
Mixed boundary conditions #879
Conversation
Ok I think this is the way to go. With last commit there absolutely no changes for everything that uses For |
Great PR! It seems that the approach is very clean! |
Is there anything I'm missing concerning places where periodicity is used to compute things? I tried grepping my way through the package but didn't see anything obvious. |
you are right, the nearby searches for GridSpaces will be surely need to be extended, since the filtering which is done for finding the right nearby positions should be done differently |
Codecov Report
@@ Coverage Diff @@
## main #879 +/- ##
==========================================
+ Coverage 71.88% 72.45% +0.57%
==========================================
Files 42 42
Lines 2746 2832 +86
==========================================
+ Hits 1974 2052 +78
- Misses 772 780 +8
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Can I get any help with this? I don't understand what's going on. ##########################################################################################
# nearby_stuff with special access r::Tuple
##########################################################################################
# TODO: We can re-write this to create its own `indices_within_radius_tuple`.
# This would also allow it to work for any metric, not just Chebyshev!
function nearby_ids(pos::ValidPos, model::ABM{<:GridSpace}, r::NTuple{D,Int}) where {D}
# simply transform `r` to the Vector format expected by the below function
newr = [(i, -r[i]:r[i]) for i in 1:D]
nearby_ids(pos, model, newr)
end
function nearby_ids(
pos::ValidPos,
model::ABM{<:GridSpace},
r::Vector{Tuple{Int64, UnitRange{Int64}}},
)
@assert abmspace(model).metric == :chebyshev
dims = first.(r)
vidx = []
for d in 1:ndims(abmspace(model).stored_ids)
idx = findall(dim -> dim == d, dims)
dim_range = isempty(idx) ? Colon() :
bound_range(pos[d] .+ last(r[only(idx)]), d, abmspace(model))
push!(vidx, dim_range)
end
s = view(abmspace(model).stored_ids, vidx...)
Iterators.flatten(s)
end
function bound_range(unbound, d, space::GridSpace{D,false}) where {D}
return range(max(unbound.start, 1), stop = min(unbound.stop, size(space)[d]))
end EDIT: nvm just realised that indeed this is not working with periodic spaces; I guess I'll just ignore it? |
I think I got a decent implementation for the nearby searches; with 6 dimensions the performance for a mixed-boundary model is pretty close to that of a full periodic model. julia> bt_pure = @belapsed nearby_positions(
$(1,5,1,5,1,5),
$(ABM(GridAgent{6}, GridSpace((5,5,5,5,5,5); metric=:euclidean, periodic=true)))
)
julia> bt_mix = @belapsed nearby_positions(
$(1,5,1,5,1,5),
$(ABM(GridAgent{6}, GridSpace((5,5,5,5,5,5); metric=:euclidean, periodic=(true,false,true,false,true,false))))
)
pure: 1.483567134268537e-8
mix: 1.7302908726178538e-8 I guess AStar is the only missing thing now |
To me seems good, but will take an in-depth look soon, add a changelog entry anyway :-) |
There's one of the tests in the AStar (L176 in astar_tests.jl) which I marked as wrong, but after some further testing I realized that I don't understand what the expected behavior is supposed to be. I wonder if there is a bug somewhere else? julia> space = ContinuousSpace((5., 5.); periodic=true)
periodic continuous space with [5.0, 5.0] extent and spacing=0.25
julia> pf = AStar(space; walkmap=trues(10,10))
A* in 2 dimensions, periodic, diagonal, ϵ=0.0, metric=DirectDistance
julia> model = ABM(ContinuousAgent{2,Float64}, space; properties=(pf=pf,))
StandardABM with 0 agents of type ContinuousAgent
space: periodic continuous space with [5.0, 5.0] extent and spacing=0.25
scheduler: fastest
properties: pf
julia> a = add_agent!((0.,0.), model, (0.,0.))
ContinuousAgent{2, Float64}(1, [0.0, 0.0], [0.0, 0.0])
julia> plan_best_route!(a, [SVector(0.0, 1.0)], model.pf)
2-element SVector{2, Float64} with indices SOneTo(2):
0.0
1.0
julia> move_along_route!(a, model, model.pf, 0.1)
ContinuousAgent{2, Float64}(1, [0.0316227766016838, 0.09486832980505139], [0.0, 0.0]) When running the last command, I was expecting the agent to move to If we take a different target destination then indeed the agent is moving straight towards it, without any seemingly arbitrary deroute: julia> a = add_agent!((0.,0.), model, (0.,0.))
ContinuousAgent{2, Float64}(2, [0.0, 0.0], [0.0, 0.0])
julia> plan_best_route!(a, [SVector(1.0, 1.0)], model.pf)
2-element SVector{2, Float64} with indices SOneTo(2):
1.0
1.0
julia> move_along_route!(a, model, model.pf, 0.1)
ContinuousAgent{2, Float64}(2, [0.07071067811865477, 0.07071067811865477], [0.0, 0.0]) |
Regarding my last comment, atm I just made the pathfinder test consistent with what the library is currently doing. Whether that is correct or not is a different topic. I implemented all the review comments (thanks for that @Tortar), and I think we have everything now |
note that you need to run collect otherwise you just compare the creation of the iterators |
Great work anyway :-) (But this still need the changelog entry) |
You are right :s then there is some noticeable performance difference (1.5e-7 s and 5e-7 s on my machine with the above benchmark in 6 dimensions). Not as bad as I initially thought it would be, but not sure it can get much better. Changelog was done btw: b2136fc |
it's okay since in the end for the majority of points the time should be the same, on the boundary this perf difference seems legit |
Closes #828.
WIP, there are still a lot of things to fix.
I tried to represent space periodicity with a new type
Periodic{D}
to only be used internally, which would just wrap a SVector with a Bool value for each spatial dimension. When given aBool
e.g.Periodic{D}(true)
it is internally converted to a SVector asfill(true, SVector{D})
.I don't have a smart way to fall back to "full periodic" and "non periodic" via dispatch yet, so as it is now every time distances have to be evaluated, each dimensions needs to be checked for whether or not it is periodic. In low-dimensional systems (which I assume make up the vast majority of use cases) there is no performance difference; in high-dimensional ones it might start to become noticeable (although I don't know to what extent this is really an issue...)
Low-dimensional space:
High-dimensional space:
An alternative approach (probably better and possibly requiring less work) is to not have a
Periodic
type, just let spaces accept whatever is passed to them (be it a Bool, a tuple an svector etc...). If it's found to be a Bool, then they will just behave as usual, but if they are passed an iterable type they will do the checking.