Hardcoded types: bug adding a new model type

[omerchor]

model_accessing_API.jl has explicit references to StandardABM, EventQueueABM and ReinforcementLearningABM in the Dict/Vec/StructVecABM unions defined at the top of the file. This means that users wanting to implement new types of ABMs, which may still have a Dict/Vector/StructVector as a container, must override nextid and add_agent_to_container!(agent::AbstractAgent, model::ABM), though the implementation is essentially a copy-paste from existing code. Specifically, if add_agent_to_container! is not manually implemented (which isn't required by the documentation, nor does it raise an notimplemented error), adding more than one agent fails if a dictionary is used as the container, because maxid is not updated

   function add_agent_to_container!(agent::AbstractAgent, model::ABM)
    ...
    # Update maxid for DictABM
    if model isa DictABM
        maxid = getfield(model, :maxid)
        if maxid[] < getid(agent)
            maxid[] = getid(agent)
        end
    end

Similarly, in collect.jl, lines 376, 404 and 431 explicitly refer to EventBasedABM, instead of calling discretimeabm(model), which would allow other implementations of continuous time models beyond EventBasedABM.

Minimal Working Example

using Agents
using Random

@agent struct Ticker(NoSpaceAgent) end

struct MinimalABM{A, C <: Union{AbstractDict{Int, A}, AbstractVector{A}}, R <: AbstractRNG} <: AgentBasedModel{Nothing}
    agents::C
    rng::R
    maxid::Base.RefValue{Int}
    time::Base.RefValue{Int}
end

function MinimalABM(::Type{A}; container::Type = Dict) where {A <: AbstractAgent}
    agents = container == Dict ? Dict{Int, A}() : Vector{A}()
    return MinimalABM{A, typeof(agents), typeof(Random.default_rng())}(
        agents, Random.default_rng(), Ref(0), Ref(0)
    )
end

Agents.containertype(::MinimalABM{A, C}) where {A, C} = C
Agents.agenttype(::MinimalABM{A}) where {A} = A
Agents.discretimeabm(::MinimalABM) = true
Agents.extra_actions_after_add!(agent, model::MinimalABM) = nothing
Agents.step!(model::MinimalABM, t::Real) = model

Agents.nextid(model::MinimalABM{A, <:AbstractDict}) where {A} = getfield(model, :maxid)[] + 1
Agents.nextid(model::MinimalABM{A, <:AbstractVector}) where {A} = nagents(model) + 1

function try_add_two_agents(container::Type)
    println("=== container = $container ===")
    model = MinimalABM(Ticker; container)

    a1 = add_agent!(Ticker, model)
    println("first agent id:  ", a1.id)
    println("maxid after first add: ", getfield(model, :maxid)[])

    a2 = add_agent!(Ticker, model)
    println("second agent id: ", a2.id)
    println("-> OK, no id collision")
end

try_add_two_agents(Vector)

try_add_two_agents(Dict)

This works for the Vector but fails for the Dict:

=== container = Vector ===
first agent id:  1
maxid after first add: 0
second agent id: 2
-> OK, no id collision
=== container = Dict ===
first agent id:  1
maxid after first add: 0
ERROR: Can't add agent to container. There is already an agent with id=1

Agents.jl version v7.0.3

[Datseris]

Thanks a lot for finding out this mistake @omerchor . This was not intentional, it was just an error in the design. We shouldn't use this const StructVecABM = Union{ construction.

I think the cleanest way forwards is to replace any referneces to types such as StructVecABM, and instead use the existing agent_container function. Or better yet, define an agent_container_type(model) = typeof(agent_container(model)) and use that type for dispatch.

Are you interested to make a PR with this fix?

[omerchor]

Yes, I will.

[omerchor]

So it turns out that all three current implementations of concrete ABMs actually have containertype and agenttype defined, for example

containertype(::StandardABM{S, A, C}) where {S, A, C} = C
agenttype(::StandardABM{S, A}) where {S, A} = A

(and the implementation is identical for EventQueueABM and ReinforcementLearningABM). This makes sense - presumably any model should have agents and a container to store them. Should I modify AgentBaseModel itself?

abstract type AgentBasedModel{
                    S <: SpaceType,
                    A <: AbstractAgent, 
                    C <: Union{AbstractDict{Int, A}, AbstractVector{A}},
              } end

With

# Existing
spacetype(::ABM{S}) where {S} = S 
# The next two are new
containertype(::ABM{S, A, C}) where {S, A, C} = C
agenttype(::ABM{S, A}) where {S, A} = A

I can apply relevant changes throughout the project - it's just a few changes here and there, but this is a bigger change than originally proposed (and it breaks backward compatibility for anyone implementing other AgentBaseModels), so I thought I'd rather ask first considering that this is my first contribution to this project.

[Datseris]

Hm, I am not sure. Why do we have to modify AgentBasedModel at the abstract level? As long as the functions containertype, agenttype are defined for concrete implementations then that's all we need, right? So an alternative way to solve this would be to simply modify the existing code to not use these VectorABM types, but rather the existing functions like containertype (as it should been doing in the first place).

Do I understand this right?

I'd be reluctant making this breaking change at this point in the development cycle (I'm happy to review PRs but won't be contributing any bugfixes anymore)

[omerchor]

Yes, you're right. The only reason to make this change is to remove some code duplication, but otherwise using the existing containertype should work, so I'll do that.