Agents.jl: `all_pairs!` doesn't respect distance metric

[hbsmith]

julia> mutable struct Agent <: AbstractAgent
           id::Int
           pos::NTuple{2,Float64}
           vel::NTuple{2,Float64}
           mass::Float64
       end

julia> space = ContinuousSpace(2; periodic = true, extend = (1,1), metric=:euclidean)
2-dimensional periodic ContinuousSpace

julia> model = AgentBasedModel(Agent,space)
 AgentBasedModel with 0 agents of type Agent 
 space: 2-dimensional periodic ContinuousSpace
 scheduler: fastest

julia> model.space.metric
:euclidean

julia> pos = [(0.0, 0.0),(0.2, 0.2),(0.5, 0.5)]
3-element Array{Tuple{Float64,Float64},1}:
 (0.0, 0.0)
 (0.2, 0.2)
 (0.5, 0.5)

julia> for i in pos
           add_agent!(i,model,(0.0,0.0),1.0)
       end

julia> model.agents
Dict{Int64,Agent} with 3 entries:
  2 => Agent(2, (0.2, 0.2), (0.0, 0.0), 1.0)
  3 => Agent(3, (0.5, 0.5), (0.0, 0.0), 1.0)
  1 => Agent(1, (0.0, 0.0), (0.0, 0.0), 1.0)

julia> for (a1, a2) in interacting_pairs(model, .29, :all)       
           @show a1.id, a2.id
       end
(a1.id, a2.id) = (1, 2)

julia> for (a1, a2) in interacting_pairs(model, .3, :all)
           @show a1.id, a2.id
       end
(a1.id, a2.id) = (1, 2)  ## This is ok, the euclidean distance between Agents 1 and 2 is .28
(a1.id, a2.id) = (2, 3)  ## This shouldn't be here! The euclidean distance between Agents 2 and 3 is .42!

The problem looks like it comes from this version of the space_neighbors function...
https://github.com/JuliaDynamics/Agents.jl/blob/b7a6344fe649aa95d26d1ec8e27d1218b1c638dc/src/core/continuous_space.jl#L252
which doesn't check model.space.metric like the space_neighbors function above it, which takes in pos.

[Libbum]

Great issue! That looks precisely like the problem.

@hbsmith: both of your recent issues are quick fixes. I could do them now, but I was wondering if you'd be interested in writing some PRs for them?

[hbsmith]

@Libbum thanks for offering, but I can be a bit slow, so feel free to write the PRs if you think it'll be quick for you.

[Libbum]

Certainly. If issues are more your thing, we're very happy to receive reports as clear as yours!

[hbsmith]

Haha, thanks for the positive feedback! I'm just out here trying to understand why tests for my own code don't pass, so I'm happy to share any issues I find.

[Datseris]

Hi @hbsmith , thanks; I think you should try a PR here, you have already identified where the source is anyways. Would be nice to increase the name of contributors! ;)

We'll help.

[Libbum]

Actually, this one is not a simple fix!

@kavir1698: I think we need to filter the search before the query goes to the DB backend. Can you take a look at that? I'm not too familiar with this interface.

We may be able to do it with a pair-wise filter, but that will be more expensive I think. I can come back to this later in the evening.

[Datseris]

I thought it was just a matter of filtering the final output.

If metric is cityblock, do nothing. If metric is euclidean, filter all pairs that don't satisfy the distance. Because cityblock is larger than euclidean, it is guaranteed that all euclidean pairs are cityblock pairs, but not vice versa.

[kavir1698]

I also think we just need to add that filter statement:

  if model.space.metric == :cityblock
    return ids
  elseif model.space.metric == :euclidean
    return filter!(i -> sqrt(sum(abs2.(model[i].pos .- pos))) ≤ r, ids)
  end

[Libbum]

pos is not a part of this function. It needs to be a pair-wise filter.

[kavir1698]

That's agent's pos, isn't it?

[Libbum]

All (returned) ids against all other (returned) ids in this case. An expensive operation. I was wondering if this condition could be part of the query.

[kavir1698]

I don't see how the number of operations could be reduced if this condition is moved to the query. It would have to calculate the euclidean distance anyway. Also, it only checks all returned ids against a single position, that of the agent, if we are talking about the space_neighbors function.

[Libbum]

Yeah, you're correct. I interpreted this a little differently than ultimately what was needed. PR incoming.