Scheduler keeps track of agents in dict

[csmaxwell]

@jackiekazil

Hello! I think this would resolve #302 . I've changed the scheduler to handle a dict rather than a list. I added a next_id() method to Model to dispense unique_ids for agents. I've made WolfSheep and the tests compatible with the change, and added a test for whether the lifespan of agents with a finite life is what they should be. I haven't messed with the docs or other examples, but I could work on that if y'all'd like.

Since this is my first pull request, please let me know if I've messed anything up!

best,
Colin

[jackiekazil]

@csmaxwell if you run this line in your dev environment, it will provide you a list of all the pep8 issues, so you don't have to wait on travis to be mean. :-)

pip install flake8
flake8 . --ignore=F403,E501,E123,E128 --exclude=docs,build

Let us know when this is read for review.

[jackiekazil]

FYI https://groups.google.com/forum/#!topic/projectmesa/x2En3z9KtWQ

[coveralls]

Coverage increased (+0.9%) to 74.772% when pulling e066756 on csmaxwell:agent_dict into 6db9efd on projectmesa:master.

[csmaxwell]

@jackiekazil Thanks for the tip on flake8! I think it's ready for review. Also, thanks for giving me the heads up on possible review delays. They won't affect my research or deadlines, so no worries from my end.

[njvrzm]

(I commented initially on the issue but I think this is probably a better place)
Nice catch indeed, and I think the approach of keeping Agents in a dictionary seems like a good one, but there are some things to consider before adopting it:

• It will change the order in which Agents are called for step() in any models using the BaseScheduler's step() method. Currently they're called in the same order as they're added, but with this change they'll be called in an order that depends on the values of the Agents' unique_ids. This could be fixed by using an OrderedDict instead.
• More importantly, it will enforce the uniqueness of the unique_id each agent is created with. Currently, despite the name, nothing happens differently if unique_id is in fact not unique across the Agents of one's Model - indeed, the nature of each Agent's unique_id has no affect at all on the behavior of the model - so this change might break some existing models. I think there should at least be an exception raised if one tries to add an agent with a duplicate unique_id to a scheduler.

I think this raises interesting questions, though: why do Agents have unique_id at all? As far as I can tell, it's only currently used for logging. Is it useful to be able to specify the id, or should that be managed for the user?

[jackiekazil]

I am going to try to do a review of this over the weekend.

[jackiekazil]

(sorry for the delay of my chime in... I have one more component to my Ph.D exam that wraps up today --- I will catch up after that.)

[pgervila]

Maybe I am missing something here and if so I apologize in advance, but in order to avoid iteration over a list that is being modified, wouldn't it suffice to iterate over a COPY of the agents list in the schedule step method, instead of the original ?

Something like this

def step(self):
    """ Execute the step of all the agents, one at a time. """
    for agent in self.agents[:] :
        agent.step()

I don't expect any significant overhead from the copy operation

The dict implementation could still be a better idea ( I have to check performance-wise), but the impact of iterating over a copy would be minimum and I think it could fix the bug

[pgervila]

In any case, even if there was no bug in the list implementation, I think dicts would be a better approach. Removing an agent from a list would be at least an order of magnitude slower than from a dict. Therefore, for models with some few thousand limited life-span agents , dicts would really make a difference

[rht]

Memory consumption is reduced in the dict version, since the list version loops
over a list copy of all the agents, while the dict version loops over the list
of the shuffled keys (there shouldn't be any runtime perf penalty).

[rht]

I have a local branch which uses @csmaxwell's PR. To speed up the process of this being merged, if it is okay, I have re-organized the commits + cleaned + reworded + rebased.

[dmasad]

This was refactored into #386