-
Notifications
You must be signed in to change notification settings - Fork 125
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
Emphasize default scheduling behavior and consequences for adding Agents #813
Comments
Ohh, didn't notice this behaviour, thanks for putting the time to investigate that! A general idea would be changing the internals by some extent, but I don't think this is better than a simple The changing would be adding agents to a vector which can be read by the scheduler so that before the next step the new agents are scheduled without the need to run collect on all the previously added agents. We manage removals by counting how many times |
It is mentioned in the docs: https://juliadynamics.github.io/Agents.jl/stable/api/#A-note-on-iteration-1 The question is, how often do you think this should be mentioned? Because I would think that saying
is already enough to highlight that the iterator is the key sequence of a dictionary and hence all "pitfalls" that apply to dynamic iterators apply here as well.
No. Not only there is no reason to do this, because once basic understanding is in place this becomes obsolete, but it also causes performance defecits.
No, because the iterator works exactly as intended so there is nothing to change.
No, because the dynamic schedulers are faster and I am very, very certain, that the overwhelming majority of users would want the default behavior to be the fastest one. Overall, the important thing to keep is mind is a phrase I always use when giving workshops on writing documentation: weight every sentence in the docs with gold. And I do not think that this is a majorly undocumented issue to justify adding several new documentation all over the place, and does not call for changing any source code. Where I find a point of agreement is to add only an extra sentence in the fastest scheduler docstring that says "this scheduler is dynamic, see "a note in iteration" in the documentation for limitations this may bring." |
Is your feature request related to a problem? Please describe.
Problem: It's not obvious that the default behavior of Agents.jl is that when you add an Agent to your model, it may or may not be activated on the step that it's added.
The reason it's not predictable is because the default scheduler (fastest) returns an iterator, and if you add an agent and it happens to be added in the KeySet downstream of where you are in the
for id in activation_order
loopAgents.jl/src/simulations/step.jl
Line 61 in 7bd1214
This seems like "intended" behavior to some degree, since it's documented that schedules can be iterators, and the default scheduler happens to be an iterator. But it's far from obvious for someone building a model under mostly non-advanced conditions why they might sometimes experience new agents being activated on the step they're added, and sometimes not.
While the docstring for the fastest scheduler says that the agents are activated in an arbitrary sequence...
...it's not mentioned that not only is it arbitrary, but that is actually is an iterator that's updated in-place while stepping (a bad practice which is easy enough to avoid if you realize it's happening).
This is also not explicitly mentioned in the docs on scheduling, or on adding agents. I only discovered the "A note on iteration" (https://juliadynamics.github.io/Agents.jl/stable/api/#A-note-on-iteration-1) while diving through closed issues relevant to this issue. See e.g. #193, where the last commenthttps://github.com//issues/193#issuecomment-609061114 is not necessarily correct. And see: #457.
It seems like most (all?) the other built-in schedulers call
collect
at some point, so I think this is only relevant to fastest?I think this is also only relevant to adding agents, as removing them shouldn't cause any issues since the iterator updates in-place.
Describe the solution you'd like
At the very least, it would be good to emphasize/warn in the docstrings and docs that adding an Agent will cause unpredictable activation under the default scheduler of fastest, and to emphasize that the fastest returned not just an arbitrary ordering, but one that gets updated in-place during activation of agents during a model step.
A better solution would be to somehow make sure that the behavior for adding agents is both predictable and documented, even when the scheduler is an iterator. I don't have any elegant ideas of how to do the goal is to avoid
collect
ing the agent ids when the scheduler is an iterator.Describe alternatives you've considered
Another alternative is to not default to a scheduler which is an iterator over the agents, especially when it's within the realm of reason that default users could be adding agents during steps.
The text was updated successfully, but these errors were encountered: