Agents.jl: Further Nearest Neighbor checks

[Libbum]

Previous discussions #300, #302

@Libbum @Datseris So the implementation is by-design, even though the doc string for interacting_pairs(:nearest) says:
"Each agent can only belong to one pair, therefore if two agents share the same nearest neighbor only one of them (sorted by id) will be paired." ?
https://github.com/JuliaDynamics/Agents.jl/blob/46effae1898e8fda7afecf272cdc25eccbda683f/src/spaces/continuous_space.jl#L348

_Originally posted by @hbsmith in https://github.com/JuliaDynamics/Agents.jl/pull/302#issuecomment-697081460_

Separately but related, I'm still not sure how these two tests
https://github.com/JuliaDynamics/Agents.jl/blob/46effae1898e8fda7afecf272cdc25eccbda683f/test/continuousSpace_tests.jl#L141
https://github.com/JuliaDynamics/Agents.jl/blob/46effae1898e8fda7afecf272cdc25eccbda683f/test/continuousSpace_tests.jl#L144
are passing, because when I try to replicate the preceding code in the REPL, I get three pairs for the first test (:scheduler) and two for the second (:nearest)

_Originally posted by @hbsmith in https://github.com/JuliaDynamics/Agents.jl/pull/302#issuecomment-697142579_

---

We need to make sure that the NN fix still satisfies our documentation and / or alter it in a meaningful manner.

Additionally, should this function obtain a new name under the same premise as #317 or is it fine as-is?

[Libbum]

I think your first point definitely requires some alterations @hbsmith. I won't have time today until the afternoon to look into this though.

[Libbum]

The problem resides in true_pairs! at the moment we only look into the first sorted index rather than both indices. There is a complexity to that choice and aligning the result with the documentation. I'll write up an explanation later on today, and hopefully we can identify a clearer sense of how to move past some contradictions here.

[Datseris]

I'd suggest to leave this until _after_ we have a new performant continuous space. who knows what changes might be necessary...?

[Libbum]

This is still a conceptual issue rather than anything to do with the space. It's fine to design now at least.

The meeting I'm in is not so relevant, so I can perhaps write out the problem in parallel.

| pair | distance |
|------|----------|
|(1, 2)|1.242966427823642|
|(2, 3)|0.8022463468445246|
|(3, 4)|1.2502774168592417|

:nearest: agents are only paired with their true nearest neighbor
(existing within radius r).
Each agent can only belong to one pair, therefore if two agents share the same nearest
neighbor only one of them (sorted by id) will be paired.

If we take that verbatim, sorting by ID means this function should return (1,2); (3,4). That's easy enough to fix, we just add the pair check similar to the :scheduler method.

Perhaps though, since (2,3) is the 'nearest, nearest neighbor', then maybe the docstring should be changed to read

if two agents share the same nearest neighbor only one of them (sorted by distance, then by id) will be paired.

And as a consequence there's only one pair returned: (2,3).

[Datseris]

I'll post here when I get some extra time, sorry that I've forgot!

[Datseris]

Okay here is my consencous after revisiting this. Firstly, the methods :nearest and :scheduler are too much and should be merged into one method that always scans by the order returned by the scheduler. Secondly, I agree with Tim's suggestion of modifying the :nearest case to do

if two agents share the same nearest neighbor only one of them (sorted by distance, then by id) will be paired.

but only if this is very easy to do in code. In general we shouldn't spend too much effort here. If the above is not easy to do, we go with the verbatim suggestion:

If we take that verbatim, sorting by ID means this function should return (1,2); (3,4). That's easy enough to fix, we just add the pair check similar to the :scheduler method.

Lastly, the docstring of the function for the case :types is inaccurate when it states scheduler = [a.id .... It should be scheduler(model) = ... as the scheduler needs to be a function.

[Libbum]

but only if this is very easy to do in code

pretty sure I already had it implemented. Will have to dig it up.