JuliaDynamics / Agents.jl

Agent-based modeling framework in Julia
https://juliadynamics.github.io/Agents.jl/stable/
MIT License
725 stars 117 forks source link

Fix bug in pos2cell #857

Closed Tortar closed 1 year ago

Tortar commented 1 year ago

This should fix the bug described in #853.

Maybe it is good if we put the code in the issue with which this was found inside a test.

@mastrof can you please confirm that this solves the problem with your code?

Tortar commented 1 year ago

This is a little breaking though since I think that anyway if the agent is at an integer position inside the space, it will change the grid position it occupies which in turn would make the approximate nearby search (the one when calling nearby_ids) possibly return different agents, this is a very rare case though, and the function is an approximation anyway and we are going for 6.0 so...

alternatively we can use the previous way with a similar transformation, but I think it would be a little less nice and a little less fast, let me know your opinion

codecov-commenter commented 1 year ago

Codecov Report

Merging #857 (1c68442) into main (62bd2a9) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #857   +/-   ##
=======================================
  Coverage   70.82%   70.82%           
=======================================
  Files          42       42           
  Lines        2787     2787           
=======================================
  Hits         1974     1974           
  Misses        813      813           
Files Changed Coverage Δ
src/spaces/continuous.jl 93.25% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

Tortar commented 1 year ago

this is the same but without the little change in behaviour

pos2cell(pos::ValidPos, model::ABM) = Tuple(min.(floor.(Int, pos./abmspace(model).spacing) .+ 1, size(abmspace(model).grid)))
Datseris commented 1 year ago

also, this returns a Tuple? Shouldnt it return an SVector.?

Tortar commented 1 year ago

no, since we need it for the gridspace

Datseris commented 1 year ago

we should measure the performance regression this has. If it is really impactful, we can honestly consider if simply not fixing #853 and accept that the edge case bug will forever exist.

Tortar commented 1 year ago

this could well even be just random variations

# median times 10000 runs
#before
Agents.jl step just moves 200 agents around (ms): 0.849313
#after
Agents.jl step just moves 200 agents around (ms): 0.855405

we can live with them I think :D

mastrof commented 1 year ago

My code seems to run alright with this fix