model.get_agents_of_type() still contains agents that have been removed both from the grid and the scheduler

[rht]

Describe the bug

I was trying to replace the lines in the Sugarscape G1MT example https://github.com/projectmesa/mesa-examples/blob/34e92d2c29a99e950bc17f7cac860c5b13156b21/examples/sugarscape_g1mt/sugarscape_g1mt/model.py#L146-L149

with

        return list(self.get_agents_of_type(Trader).shuffle(inplace=True))

This failed the tests, because when an agent dies, it only removes itself from the grid and the scheduler.

Expected behavior

The users would expect the 2 methods to give the same outcome, but will be tripped by the new requirement of agent.remove().

[quaquel]

I am not familiar with the inner details of Sugarscape. So, I don't fully understand what is happening. Both the code you linked to and the shown code take a set of agents and shuffle them. Neither calls another method.

The key difference is that the current code takes the agents from the scheduler, while the new code takes the agents from the model.

1. For backward compatibility, self.schedule.agents_by_type[Trader] should still work.
2. Removing from the scheduler in the current version "deactives" an agent but does not remove the agent from the model.
3. It seems you are now trying to mix new and old functionality. But new functionality insists on the use of agent.remove(). So, I don't think this is a bug but actually desired behavior. We probably should do more to communicate this clearly.

[rht]

model.get_agents_of_type() is a public API, and users wouldn't expect that it can only be used when schedulers are not present and agent.remove(). This is somewhat non-local, that you have to be aware of this gotcha, even if there is a clear documentation for it.

I'd say, agents_by_type needs to be renamed _agents_by_type, at least until the no-scheduler version of the code has become the standard way of doing activation.

[rht]

The other alternative that I didn't say, but would make sense given the trade off, is to sacrifice the feature of having multiple schedulers by doing agent.remove() in the scheduler's remove method. get_agents_of_type/agents_by_type usage is much more common than having multiple schedulers, and should be preferred.

[quaquel]

model.get_agents_of_type() is a public API, and users wouldn't expect that it can only be used when schedulers are not present and agent.remove().

I am not sure whether I agree with this. I think conceptually it is fine to say agent.remove removes the agent from the model, while scheduler.remove removes the agent from the scheduler but not from the model.

[Corvince]

I am also not sure what exactly the problem is here, but from the title I don't see how this wouldn't be expected behavior. Why should agents be removed from model.agents if they are removed from schedule and grid? This just represents passive agents, so there is even a use case for this behavior. Think of a board game where playing pieces are taken of the board, but might reappear later.

The inverse could be expected: Removing an agent from model.agents should also remove it from the scheduler and the grid. So in the example above, taken completely out of the game. And I think thats also the intention of using weakrefs.

[quaquel]

Exactly. The reason for using weakrefs in AgentSet was to make it easy to remove agents from everywhere by just calling agent.remove. Clearly this vision is not yet fully realized because we don't use agentset or weakrefs yet in the spaces.

And, of course, we are discovering various performance drawbacks of weakrefs, which we then try to work around to maintain performance.

[rht]

Once again,

The users would expect the 2 methods to give the same outcome, but will be tripped by the new requirement of agent.remove().

Please see it from the user perspective. It may be obvious to Mesa developers because you implemented the feature and the cause of the examples tests failing had been debugged during #1942. But the user who using the new method get_agents_of_type wouldn't necessarily know the inner working and the nuances.

[quaquel]

I agree with you on this point which is why we need to carefully check the documentation to make clear what the behavior is of these different forms of removing agents.

[EwoutH]

Now we have updated to direct acces to an agents_by_type dict property in #2267, does this issue still persists?