Remove ValidPos type

[simsurace]

This is an attempt at addressing #1094.
There are remaining ambiguities that need to be resolved:

Tuple{Method, Method}[
    (
        add_agent!(model::AgentBasedModel, args::Vararg{Any, N}; kwargs...) where N @ Agents ~/.julia/dev/Agents/src/core/space_interaction_API.jl:260,
        add_agent!(pos, A::Union{Function, Type}, model::AgentBasedModel, args::Vararg{Any, N}; kwargs...) where N @ Agents ~/.julia/dev/Agents/src/core/space_interaction_API.jl:281
    ),
    (
        add_agent!(agent::AbstractAgent, pos, model::AgentBasedModel) @ Agents ~/.julia/dev/Agents/src/core/space_interaction_API.jl:220,
        add_agent!(pos, A::Union{Function, Type}, model::AgentBasedModel, args::Vararg{Any, N}; kwargs...) where N @ Agents ~/.julia/dev/Agents/src/core/space_interaction_API.jl:281
    ),
    (
        kwcall(::NamedTuple, ::typeof(add_agent!), model::AgentBasedModel, args::Vararg{Any, N}) where N @ Agents ~/.julia/dev/Agents/src/core/space_interaction_API.jl:260,
        kwcall(::NamedTuple, ::typeof(add_agent!), pos, model::AgentBasedModel, args::Vararg{Any, N}) where N @ Agents ~/.julia/dev/Agents/src/core/space_interaction_API.jl:270
    ), 
    (
        add_agent!(agent::AbstractAgent, pos, model::AgentBasedModel) @ Agents ~/.julia/dev/Agents/src/core/space_interaction_API.jl:220,
        add_agent!(pos, model::AgentBasedModel, args::Vararg{Any, N}; kwargs...) where N @ Agents ~/.julia/dev/Agents/src/core/space_interaction_API.jl:270
    ),
    (
        kwcall(::NamedTuple, ::typeof(add_agent!), model::AgentBasedModel, args::Vararg{Any, N}) where N @ Agents ~/.julia/dev/Agents/src/core/space_interaction_API.jl:260,
        kwcall(::NamedTuple, ::typeof(add_agent!), pos, A::Union{Function, Type}, model::AgentBasedModel, args::Vararg{Any, N}) where N @ Agents ~/.julia/dev/Agents/src/core/space_interaction_API.jl:281), (add_agent!(model::AgentBasedModel, args::Vararg{Any, N}; kwargs...) where N @ Agents ~/.julia/dev/Agents/src/core/space_interaction_API.jl:260, add_agent!(pos, model::AgentBasedModel, args::Vararg{Any, N}; kwargs...) where N @ Agents ~/.julia/dev/Agents/src/core/space_interaction_API.jl:270
    )
]

[Datseris]

yeah as I mentioned on Slack, add_agent! is the main problem. I don't say a way to resolve these ambiguities at the moment.

[Datseris]

This is an opportunity to also improve these functions:

4. Extend random_position(model).
5. Extend add_agent_to_space!(agent, model), remove_agent_from_space!(agent, model).

Instead of taking the model as a second argument, they should take the space instead, so that we have a top level dispatch to abmspace(model) and then spaces extend the methods more directly. As far as I remember, all of these methods don't need the model, they need the space instead.

[Datseris]

thanks for the PR @simsurace . I would suggest that we use this PR as a general "improve experience of implementing new space downstream". It's up to you though, if you don't want to put the time and only want to fix the ValidPos, that's also fine.

[Tortar]

These ambiguities shouldn't be hard to solve, they are mainly innocuous ones if I read them correctly...strangely tests seem stuck though

[Tortar]

I think we should extend any function related to space that way, actually we could maybe use f(..., space, model) instead of f(..., space) if there are functions which require the model for API coherency

[simsurace]

To me it's not clear how to resolve ambiguities in a way that does not break the previous extension protocol for new spaces.
One could just define one top-level method that includes all conflicting signatures and has logic flow based on their types. This would still be lowered to the same thing more or less, but is not very nice in terms of readability.
If someone wants to give a shot, feel free to continue the PR.

[Datseris]

to me its not clear either how to resolve the add_agent! ambiguities. I don't think it is possible actually. I think we need to redesign add_agent! for it to be possible. Or to redesign the whole concept of ValidPos.

@simsurace , out of curiosity, what prevents you from adding your space directly to this repo?

[Tortar]

I will give it a shot, let's see

[Tortar]

I fixed all the remaining ambiguities, they were all invalid signatures, so they were fortunately innocuous as I initially thought

[simsurace]

@simsurace , out of curiosity, what prevents you from adding your space directly to this repo?

It is supposed to be possible to define new user-defined spaces. While some spaces a user may want to define may be of general interest and deserve upstreaming, I'm sure there are a lot that do not warrent being added to and supported by Agents.jl.

[Tortar]

the 2 tests that do not pass seem like wrong tests

[Tortar]

Thank you a lot @simsurace for starting this, it was clearly something really needed

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 78.57143% with 6 lines in your changes missing coverage. Please review.

Project coverage is 85.48%. Comparing base (8b5b456) to head (817a7f0).
Report is 149 commits behind head on main.

Files with missing lines	Patch %	Lines
src/ambiguities.jl	0.00%	4 Missing ⚠️
src/core/space_interaction_API.jl	50.00%	1 Missing ⚠️
src/spaces/grid_multi.jl	80.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1093       +/-   ##
===========================================
+ Coverage   70.12%   85.48%   +15.36%     
===========================================
  Files          42       37        -5     
  Lines        2718     2536      -182     
===========================================
+ Hits         1906     2168      +262     
+ Misses        812      368      -444     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.