Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing type signature in remove_all_from_space! method for discrete space? #1104

Closed
simsurace opened this issue Nov 14, 2024 · 5 comments · Fixed by #1105
Closed

Missing type signature in remove_all_from_space! method for discrete space? #1104

simsurace opened this issue Nov 14, 2024 · 5 comments · Fixed by #1105
Labels
bug Something isn't working space interaction api About how a simulation interacts with the space

Comments

@simsurace
Copy link
Contributor

simsurace commented Nov 14, 2024

Is this method supposed to be a generic fallback or not? If not (as I would guess) it should probably be restricted to model::ABM{<:DiscreteSpace}.

function remove_all_from_space!(model)
for p in positions(model)
empty!(ids_in_position(p, model))
end
end

When defining a new space, and absent this fallback, it seems like the mandatory interface to be defined would also include remove_all_from_space!. However, this is currently not listed in the docs. Therefore, when I implemented a new space and did not define a method for this function, I hit the method above.

@Datseris
Copy link
Member

Datseris commented Nov 15, 2024

No, remove_all_from_space! should not be a mandatory method. What is the trace? Where is it called? The generic fallback for remove_all! shoould be a loop for a in allagents(model); remove_agent!(a, model); end so I don't see why remove_all_from_space! even exists.

And yes, definitely the above method is only valid for discrete spaces.

@simsurace
Copy link
Contributor Author

Maybe I'm missing something. This is what happens:

using Agents

struct DummySpace <: Agents.AbstractSpace
    ids::Vector{Int}
end

DummySpace() = DummySpace(Int[])

Agents.random_position(::ABM{<:DummySpace}) = nothing

function Agents.add_agent_to_space!(agent::Agents.AbstractAgent, model::ABM{<:DummySpace})
    space = Agents.abmspace(model)
    push!(space.ids, agent.id)
    return agent
end

function Agents.remove_agent_from_space!(agent::Agents.AbstractAgent, model::ABM{<:DummySpace})
    space = Agents.abmspace(model)
    ai = findfirst(i -> i == agent.id, space.ids)
    deleteat!(space.ids, ai)
    return agent
end


@agent struct DummyAgent(NoSpaceAgent)
    pos::Nothing
end

model = StandardABM(DummyAgent, DummySpace(); agent_step! = (a, m) -> a)

add_agent!(DummyAgent, model)
add_agent!(DummyAgent, model)
add_agent!(DummyAgent, model)

remove_all!(model)

This is where the wrong method is hit:

ERROR: MethodError: no method matching positions(::DummySpace)

Closest candidates are:
  positions(::AgentBasedModel{<:Union{Agents.OSM.OpenStreetMapSpace, GraphSpace}})
   @ Agents ~/.julia/packages/Agents/W7Wee/src/spaces/utilities.jl:180
  positions(::AgentBasedModel{<:Agents.DiscreteSpace}, ::Symbol)
   @ Agents ~/.julia/packages/Agents/W7Wee/src/spaces/discrete.jl:30
  positions(::AgentBasedModel)
   @ Agents ~/.julia/packages/Agents/W7Wee/src/spaces/discrete.jl:18
  ...

Stacktrace:
 [1] positions(model::StandardABM{…})
   @ Agents ~/.julia/packages/Agents/W7Wee/src/spaces/discrete.jl:18
 [2] remove_all_from_space!(model::StandardABM{…})
   @ Agents ~/.julia/packages/Agents/W7Wee/src/spaces/discrete.jl:305
 [3] remove_all!(model::StandardABM{…})
   @ Agents ~/.julia/packages/Agents/W7Wee/src/core/space_interaction_API.jl:154
 [4] top-level scope
   @ ~/test.jl:35
Some type information was truncated. Use `show(err)` to see complete types.

@Datseris
Copy link
Member

https://github.com/JuliaDynamics/Agents.jl/blob/main/src/core/space_interaction_API.jl#L153-L157

Right, I see now. We've created this for performance, as it is faster to remove all agents at once rather than looping over and removing each individual agent.

However, the function remove_all_from_space! should have had a generic implementation:

function remove_all_from_space!(model)
    for a in allagents(model); remove_agent_from_space!(a, model); end
end

Do you mind putting in a quick PR?

@Datseris
Copy link
Member

In the dev docs under the section of optional extensions teh function remove_all_from_space! can be mentioned.

@simsurace
Copy link
Contributor Author

Sure, I'll get it done soon!

@Tortar Tortar added bug Something isn't working space interaction api About how a simulation interacts with the space labels Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working space interaction api About how a simulation interacts with the space
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants