JuliaDynamics / Agents.jl

Agent-based modeling framework in Julia
https://juliadynamics.github.io/Agents.jl/stable/
MIT License
717 stars 115 forks source link

Reconsider the deprecation of adding agents with an agent instance #945

Closed Tortar closed 6 months ago

Tortar commented 7 months ago

This post https://discourse.julialang.org/t/swap-agent-positions-in-agents-jl/107774/2 made me realize that we don't want to deprecate the adding of agents by passing an instance to add_agent!, if anything, we should instead deprecate the add_agent! function which passes args and kwargs.

The user in that thread wanted to remove the agent and then re-add it after some changes, the problem is that as of 6.0 this can't be done, since add_agent! requires passing all args except the id, this causes a much further complicated syntax, but not only that, it can't actually be done since we call nextid inside add_agent! effectively changing the agent. In general the first reference of the agent is lost (the one before add_agent!) and so it is a struggle to do this sort of things...

I think all of this problem can be reduced to the fact that add_agent! does more that what it should, it doesn't just add the agent to the model, but also creates it, which causes problems.

But there is a better solution without changing the fact that we handle the nextid field, we can just exploit the fact that we are using a macro to create the agent, adding an inner constructor like so:

@agent Foo(NoSpaceAgent)
    x::Int
end

creates

@kwdef mutable struct Foo <: AbstractAgent
    const id::Int
    x::Int
end

# methods added automatically
Foo(model::ABM, args...) = Foo(Agents.nextid(model), args...)
Foo(model::ABM; kwargs...) = Foo(; id = Agents.nextid(model), kwargs...)

Those definitions already work:

julia> model = StandardABM(Foo)

julia> Foo(model, 2)
Foo(1, 2)

julia> Foo(model; x = 2)
Foo(1, 2)

Notice that we need to pass the model as the first argument otherwise we have problems with dispatch, but I think it's okay to do so.

With this, I think that instead of deprecating add_agent!(agent, model) and add_agent_pos!(agent, model) we could deprecate the other versions of add_agent!.

cc @Datseris

Datseris commented 7 months ago

The user in that thread wanted to remove the agent and then re-add it after some changes

As for the issue linked, we have a public API function to swap agents which is what this discourse post wants.

I think all of this problem can be reduced to the fact that add_agent! does more that what it should, it doesn't just add the agent to the model, but also creates it, which causes problems.

We are now in version 6 with thousands of users of Agents.jl and I've never heard of these problems. What problems is it causing? Can you be explicit?

does more that what it should

Are these two different things actually different? Not really. The only reason to create an agent is to add it to the model. Agents outside the model have no reason to exist within the context of Agents.jl. At least as far as Agents.jl is concerned, creating an Agent.jl and not adding it to the model is pretty much equivalent to not creating an agent at all.

With this, I think that instead of deprecating add_agent!(agent, model) and add_agent_pos!(agent, model) we could deprecate the other versions of add_agent!.

This is massively breaking and a humongous change, and honestly feels rather a rushed decision motivated by an issue that is a no factor in my eyes as an API function already exists to do what the issue wants to do.


All in all I strongly disagree with the proposal and would need much stronger arguments to remove one of the main convenience features of Agents.jl that the other competing frameworks do not seem to even have.

In any case, adding custom constructors so that we allow manual creation of Agents.jl is something I am okay with. Note that I still see no reason for it but if others do they may as well use it.

Tortar commented 7 months ago

We are now in version 6 with thousands of users of Agents.jl and I've never heard of these problems. What problems is it causing? Can you be explicit?

(Almost) nobody is using version 6, it is unreleased

Tortar commented 7 months ago

yeah, I mean I'm okay to have still all versions of add_agent!, I'm just saying that deprecating add_agent!(agent, model)and add_agent_pos!(agent, model) is wrong because otherwise you can't remove and re-add an agent afterwards, yes, in this case it is about swapping agents where in v6 there is a function for it, but there are other cases where one would like to do so, and this is not possible with the other versions of add_agent!. Just providing those constructor should be fine though.

So surely I agree, no need to remove those convenience functions in the end, but we need to give again the possibility to use the normal add_agent! with an agent instance

Tortar commented 7 months ago

mmmh, but yes it is more about that and less about the new constructor methods...so we can just reinsert add_agent!(agent, model) and add_agent_pos!(agent, model) in the public api and that's it

Datseris commented 7 months ago

I'm just saying that deprecating add_agent!(agent, model)and add_agent_pos!(agent, model) is wrong because otherwise you can't remove and re-add an agent afterwards, yes, in this case it is about swapping agents where in v6 there is a function for it, but there are other cases where one would like to do so, and this is not possible with the other versions of add_agent!.

We have to be careful here.

  1. Remember why we deprecated this in the first place. We had reasons. They were about allowing us to have "privacy" over possibly internal agent fields.
  2. The proposed solution here is a bit fishy in my opinion. We are defining constructors that initialize unknown fields. This can get weird. If I was allowed to make my own agents, I would expect I wouldn't need the model instance. After all, the whole point of a type constructor is to be pure: it just takes the inputs for the type. For an agent this is not valid, as we would need a model reference to figure out what ID we need to make.
  3. My take on this whole storhy is that first we are trying to solve a problem that doesn't exist yet: what are these other cases where a user would need to remove and re-add an agent to the model? this doesn't make much sense to me. Are we sure that such cases exist? And if they do, maybe there is a generic situation that can simply become an API function? It really isn't hard to add an API function into Agents.jl. Look at the example of the swap_agents function. It was 5 lines of code.

Let's also be consider this for a moment: a user that is so advanced that wants to make their own agents by hand (for whatever reason not yet justified) may as well go ahead and read the source code and realize it is as simply as calling the _to_model and _to_space functions individually. So we could easily live without every allowing add_agent!(agent, and just acknowledge that aAgents.jl is open source and fuilly hackable anywyas. Already someone can add manually agents if they want to.

Tortar commented 7 months ago

In my opinion the main "mistake" is conflating the creation of agents with the add of agents to the model. While the user in that thread has a solution also in v6 because we implemented swap_agents!, this is just a coincidence, if this didn't happen the user would have needed to mess with internals, actually not that much as you say, but some people could be reluctant to use internal functions, for example because they are subject to changes in the future. So in my opinion it is okay to keep the current add_agent! and the current constructors (even if they shouldn't be used) but also to create new constructors for the agents which use the model instance + the add_agent!(agent,...) and add_agent_pos!(agent,...). The purity of the constructor is my opinion already "spoiled" by the fact that we use a macro for agent structs, we can do every sort of (good) tricks for this reason :-)

An example of removing and re-adding could be when you have a sort of intervention scheme, say you have a model with two agent types, usually just one is present and you observe the dynamics, then you want to insert the second type of agents to see the dynamics with both for some time, then remove those agents to see the "recovery" stage, and re-add them later again to see the interaction after the "recovery". This can be done in some other way even with the current api, but it is more awkward and harder, e.g by putting the second type of agents in an unreachable place inside the space of the model. Maybe this is not a very good example but it illustrates the usefulness of such a feature in my opinion.

If we don't want to add custom constructors, we can implement a new create_agent function but to me this seems overly verbose.

Anyway, we will just document this features, but keep the current way with add_agent!, so the needed changes require nearly adding just 10 lines of code

Datseris commented 7 months ago

Okay, sure, we can re introduce adding an existing agent with add_agent. As you say, it really isn't much effort or much more things in the docs.