Skip to content

Scheduler: track agents with dict instead of list #386

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

Scheduler: track agents with dict instead of list #386

wants to merge 5 commits into from

Conversation

@rht
Copy link
Contributor

@rht rht commented Jun 17, 2017 •
edited
Loading

Based on #305. Fix #302.

@coveralls
Copy link

coveralls commented Jun 17, 2017 •
edited
Loading

Coverage Status

Coverage increased (+1.0%) to 77.327% when pulling 516df80 on rht:agent_dict into 49ebfc5 on projectmesa:master.

rht
rht commented Jun 24, 2017
View reviewed changes
mesa/batchrunner.py
""" Run reporters and collect agent-level variables. """
agent_vars = {}
for agent in model.schedule.agents:
for agent in model.schedule.agents.values():
Copy link
Contributor Author

@rht rht Jun 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC the ordering is non-deterministic in python<3.6. Yet deterministic execution is crucial in scientific simulations.

Copy link
Member

@dmasad dmasad Jul 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that preserving order is important. I'm not sure whether in Python <3.6 iterating over values() is guaranteed to preserve order any better than just iterating over the keys, and/or for that order to be the same as the order in which the agents were added (which should be the default non-random behavior).

Copy link
Contributor Author

@rht rht Jul 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A sure-fire fix is to use collections.OrderedDict. But this will be slower since in Python 3 it is not written in C. But correctness > speed in this case.

Copy link
Contributor Author

@rht rht Jul 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it has to be a defaultdict + collections.OrderedDict.

rht
rht commented Jul 2, 2017
View reviewed changes
mesa/time.py
""" Execute the step of all the agents, one at a time. """
for agent in self.agents[:]:
agent.step()
agent_keys = list(self.agents.keys())
Copy link
Contributor Author

@rht rht Jul 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, in all the places where .values() are used, the order does not matter since they are for data collection/record or asserting properties. This is the only place where the order matters.
But in one way or another, order preserving still has to be enforced so that users running a Mesa simulation don't accidentally fall into the pitfall -- principle of least surprise.

@rht
Copy link
Contributor Author

rht commented Jul 7, 2017 •
edited
Loading

@dmasad, I just figured that the deterministic run problem can be resolved with PYTHONHASHSEED=0 (>=3.2.3).
I tested

PYTHONHASHSEED=0 python3.5 -c 'print({"GO:0000054": "ribosomal subunit export from nucleus","GO:0000278": "mitotic cell cycle","GO:0000746": "conjugation","GO:0000902": "cell morphogenesis","GO:0000910": "cytokinesis"})'
{'GO:0000746': 'conjugation', 'GO:0000910': 'cytokinesis', 'GO:0000278': 'mitotic cell cycle', 'GO:0000902': 'cell morphogenesis', 'GO:0000054': 'ribosomal subunit export from nucleus'}

I repeated more than 30 times, with all the result having the same order. If this were to happen by chance, the probability would be ((0.25**5)**30 = 4.9e-91 1/(5! ** 30) = 4.2e-63).

Since there is no way to set PYTHONHASHSEED from a library, the only way to do this is via os.environ['PYTHONHASHSEED'] = 0.

Edit: ugh, I forgot to power 0.25 by 5
Edit2: it should have been factorial

@rht
Copy link
Contributor Author

rht commented Jul 7, 2017

There are 3 ways to resolve this:

  1. os.environ['PYTHONHASHSEED'] = 0.
  2. Stick to list, add an attribute to the agent, self.alive. I have seen this being used in several ABM's. Caveat: if lots of agents are created/destroyed over time, the list grows, which results in memory leaks and slow loops.
  3. python>=3.6.

I am inclined with 1, since 3 makes it harder to run Mesa. The version in Debian stable as of this timestamp is still 3.5.3 https://packages.debian.org/stretch/python3, but 3.6.1 in macOS/Homebrew https://github.com/Homebrew/homebrew-core/blob/dd7a66070b32a07b31ca2cbf4b3509cad5971226/Formula/python3.rb#L4.

@dmasad
Copy link
Member

dmasad commented Jul 13, 2017

Is there a reason not to just use OrderedDicts?

@rht
Copy link
Contributor Author

rht commented Jul 13, 2017

Yes, that will do, OrderedDict and .get('key', default_value), with reduced speed as a trade-off (but this can always be fixed later, as long as the implementation is correct). I will push this.

@rht rht force-pushed the agent_dict branch 2 times, most recently from 39f6155 to 3ebae89 Compare July 13, 2017 04:23
@coveralls
Copy link

coveralls commented Jul 13, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.03%) to 82.835% when pulling 3ebae89 on rht:agent_dict into ffe5ca1 on projectmesa:master.

@dmasad dmasad mentioned this pull request Jul 14, 2017
Closed
@jackiekazil
Copy link
Member

jackiekazil commented Jul 14, 2017

/cc @csmaxwell for tracking... this is continuation of #305

@coveralls
Copy link

coveralls commented Nov 13, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.03%) to 82.162% when pulling 8c0c340 on rht:agent_dict into 275674d on projectmesa:master.

@TaylorMutch
Copy link
Contributor

TaylorMutch commented Nov 28, 2017

@rht What's the status on this one? Looks like there is a conflict (possibly just stale).

@coveralls
Copy link

coveralls commented Nov 28, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.03%) to 82.338% when pulling 19faadc on rht:agent_dict into b4be992 on projectmesa:master.

@rht
Copy link
Contributor Author

rht commented Nov 28, 2017

Yeah, I have just rebased this.

csmaxwell and others added 5 commits December 16, 2017 03:54
@csmaxwell @rht
scheduler: track agents with dict instead of list
9bb249d 
An agent's unique id is generated by via `model.next_id()`
- Update the wolf_sheep example
- Update batchrunner, datacollection to use dict
- Update the tests
@csmaxwell @rht
Add test for lifespan of agents
5790512
@csmaxwell @rht
Add call for unittest
fcf8b86
@csmaxwell @rht
Rewrite agent activation unittests
c18aa50
@rht
Replace defaultdict with OrderedDict
17ed18b
@rht
Copy link
Contributor Author

rht commented Dec 16, 2017

Rebased again.

@coveralls
Copy link

coveralls commented Dec 16, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 82.979% when pulling 17ed18b on rht:agent_dict into a6c74cf on projectmesa:master.

@dmasad dmasad self-assigned this May 4, 2018
@dmasad dmasad mentioned this pull request May 10, 2018
Merged
@dmasad
Copy link
Member

dmasad commented May 13, 2018

Closing this one since it's incorporated into #510

@dmasad dmasad closed this May 13, 2018
@jackiekazil jackiekazil added this to the Unknown milestone milestone Nov 22, 2020
@rht rht deleted the agent_dict branch September 16, 2023 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmasad dmasad dmasad left review comments

Assignees

@dmasad dmasad

Labels

None yet

Projects

None yet

Milestone

Unknown milestone

Development

Successfully merging this pull request may close these issues.

Removing an agent interferes with RandomActivation step method

6 participants

@rht @coveralls @dmasad @jackiekazil @TaylorMutch @csmaxwell