Skip to content
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

track unique_id automatically #2260

Merged
merged 34 commits into from
Sep 4, 2024
Merged

track unique_id automatically #2260

merged 34 commits into from
Sep 4, 2024

Conversation

quaquel
Copy link
Member

@quaquel quaquel commented Aug 30, 2024

This PR ensures that the unique_id in Agent is assigned automatically. It draws on work done in #2226. In short, this PR makes 3 changes:

  • Agent.unique_id is assigned automatically and no longer used as an argument to Agent. A deprecation warning is issued if the Agent is instantiated with 2 arguments (unique_id and model). The value passed for unique_id is ignored in favor of the new system. This last point is in line with Automatically assign unique_id to Agents #2226.
  • Model.next_id() raises a deprecation warning and always return 0.
  • unique_id is unique relative to a Model instance. Internally, Agent has a class-level attribute _ids. This attribute is a dictionary with the Model instance as key and the "generator" for unique_id as value.

All unit tests and benchmarks work with the new structure. They raise no deprecation warnings.

examples, tutorials, etc., still need to be updated to be consistent with this change.

Resolves #2213.

Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔴 +72.1% [+69.9%, +74.2%] 🔵 +0.5% [+0.4%, +0.6%]
BoltzmannWealth large 🔴 +60.9% [+21.5%, +108.1%] 🔴 +6.0% [+3.1%, +8.7%]
Schelling small 🔴 +23.7% [+23.3%, +24.0%] 🔵 +1.4% [+1.1%, +1.7%]
Schelling large 🔴 +27.9% [+27.3%, +28.8%] 🔵 +5.9% [+3.0%, +9.0%]
WolfSheep small 🔴 +37.4% [+36.3%, +38.3%] 🔴 +7.8% [+7.5%, +8.0%]
WolfSheep large 🔴 +37.1% [+36.3%, +37.9%] 🔴 +11.7% [+8.9%, +14.0%]
BoidFlockers small 🔴 +33.0% [+31.8%, +34.2%] 🔵 +3.1% [+2.4%, +3.8%]
BoidFlockers large 🔴 +33.5% [+32.0%, +34.9%] 🔵 +3.7% [+3.0%, +4.2%]

mesa/agent.py Outdated Show resolved Hide resolved
@Corvince
Copy link
Contributor

What I don't understand, but unfortunately can't test right now: doesn't this keep increasing the counter indefinitely, even across models? So if I run the same model twice without restarting my kernel, won't the ids be different?

mesa/agent.py Outdated Show resolved Hide resolved
@EwoutH
Copy link
Member

EwoutH commented Aug 30, 2024

I would like all maintainers to check our Matrix chat before moving further.

@quaquel
Copy link
Member Author

quaquel commented Aug 31, 2024

I continued some work on this, mainly to understand how breaking automatic counting actually is. And it indeed creates a massive mess throughout the various tests, including very old test code that probably has not been touched in a long while (e.g., the test code of the schedulers).

I did 3 things here

  • _ids has become essentially a defaultdict with the model as key and the intertools.count(1) as value. This means that (a) unique_id is unique relative to the model (so option 2 above) and (b) that unique_id starts from 1 just as the old code did
  • removed model.next_id
  • made the various tests work again.

Like in #2226, The code here still accepts a unique_id and raises a warning. If you remove that, the number of tests that fails explodes again. There are many tests that use custom unique ids (so not even model.next_id.

Not sure where this leaves #2226 or this one. It seems there is broad agreement on having unique_id assigned automatically in MESA 3, but it is equally clear that this involves quite some more work depending on how breaking this new feature is implemented.

@quaquel quaquel added breaking Release notes label trigger-benchmarks Special label that triggers the benchmarking CI labels Sep 4, 2024
Copy link

github-actions bot commented Sep 4, 2024

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔴 +12.4% [+10.8%, +13.8%] 🔵 -1.0% [-1.3%, -0.7%]
BoltzmannWealth large 🔵 -9.6% [-26.2%, +6.8%] 🟢 -5.3% [-6.8%, -3.5%]
Schelling small 🔵 +1.7% [+1.3%, +2.1%] 🔵 +0.3% [+0.0%, +0.5%]
Schelling large 🔵 +1.5% [+0.6%, +2.4%] 🔵 +1.5% [+0.2%, +3.1%]
WolfSheep small 🔴 +4.3% [+3.3%, +5.1%] 🔵 +2.0% [+1.7%, +2.3%]
WolfSheep large 🔵 +3.2% [+2.7%, +3.7%] 🔵 +2.3% [+1.7%, +3.0%]
BoidFlockers small 🔵 +3.6% [+3.0%, +4.2%] 🔵 +0.2% [-0.7%, +1.0%]
BoidFlockers large 🔵 +3.6% [+2.8%, +4.3%] 🔵 -0.2% [-0.6%, +0.2%]

@quaquel quaquel removed trigger-benchmarks Special label that triggers the benchmarking CI 2 - WIP labels Sep 4, 2024
@quaquel quaquel marked this pull request as ready for review September 4, 2024 12:32
Copy link

github-actions bot commented Sep 4, 2024

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🟢 -9.5% [-10.6%, -8.5%] 🟢 -17.9% [-18.0%, -17.8%]
BoltzmannWealth large 🟢 -7.4% [-8.0%, -6.8%] 🟢 -32.0% [-34.9%, -28.3%]
Schelling small 🟢 -13.5% [-13.7%, -13.2%] 🟢 -21.5% [-21.8%, -21.3%]
Schelling large 🟢 -13.1% [-14.0%, -12.2%] 🟢 -14.8% [-15.8%, -13.9%]
WolfSheep small 🟢 -11.7% [-12.4%, -10.9%] 🟢 -14.3% [-14.6%, -14.1%]
WolfSheep large 🟢 -11.5% [-13.0%, -10.0%] 🟢 -13.7% [-15.6%, -11.7%]
BoidFlockers small 🟢 -9.4% [-9.9%, -8.9%] 🟢 -10.2% [-10.9%, -9.5%]
BoidFlockers large 🟢 -8.9% [-9.4%, -8.4%] 🟢 -13.1% [-13.7%, -12.4%]

Copy link
Member

@EwoutH EwoutH left a comment

Choose a reason for hiding this comment

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

Overall looks good, few minor things.

mesa/model.py Show resolved Hide resolved
mesa/model.py Show resolved Hide resolved
mesa/agent.py Show resolved Hide resolved
mesa/agent.py Outdated Show resolved Hide resolved
tests/test_datacollector.py Show resolved Hide resolved
@quaquel quaquel merged commit 46a8752 into projectmesa:main Sep 4, 2024
2 of 3 checks passed
@EwoutH EwoutH added this to the v3.0 milestone Sep 4, 2024
@EwoutH
Copy link
Member

EwoutH commented Sep 4, 2024

Great work, thanks!

@divilian
Copy link

I like the idea of unique_id being automatically set for an Agent, instead of having to pass it to the constructor explicitly. But the sad thing for me is that all my agents are also nodes in a networkx graph, and nodes in networkx are numbered starting from 0, not 1. So my code that worked before upgrading to Mesa 3.0 now breaks, because before I was setting all the agent IDs from 0, and now they are "helpfully" being automatically set starting from 1.

Any obvious clean way around this? Making my networkx graph "one node larger" and ignoring its node #0 to accommodate Mesa is a hack I'm not prepared to consider at this point. I'm wondering if there's any easy option to putting in a bunch of logic in my code to subtract 1 and add 1 to IDs at various points where I switch back and forth between working with "agent k" vs. "graph node k-1".

@quaquel
Copy link
Member Author

quaquel commented Dec 20, 2024

The easiest thing would be to add a @property to your Agent:

@property
def node_id(self):
    return self.unique_id - 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automate and enforce unique unique_id in Agents
5 participants