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

fix: register agent after creating unique_id and pos attributes #2368

Merged

Conversation

wang-boyu
Copy link
Member

One gis example uses agent's unique_id to compare and hash agents: https://github.com/projectmesa/mesa-examples/blob/5758b6504fb26a2af05d861f29d32a81a07200f6/gis/agents_and_networks/src/agent/building.py#L34-L40

It was working until #2328, in which the call to self.model.register_agent(self) in Agent.__init__() was moved from below self.unique_id = next(self._ids[model]) to above it.

As a result, in the gis example we now have the following error message:

File "/Users/boyu/GitHubProjects/mesa-examples/gis/agents_and_networks/src/agent/building.py", line 23, in __init__
    super().__init__(model=model, geometry=geometry, crs=crs)
  File "/Users/boyu/GitHubProjects/mesa-examples/venv/lib/python3.12/site-packages/mesa_geo/geoagent.py", line 36, in __init__
    Agent.__init__(self, model)
  File "/Users/boyu/GitHubProjects/mesa-examples/venv/lib/python3.12/site-packages/mesa/agent.py", line 65, in __init__
    self.model.register_agent(self)
  File "/Users/boyu/GitHubProjects/mesa-examples/venv/lib/python3.12/site-packages/mesa/model.py", line 145, in register_agent
    self._agents[agent] = None
    ~~~~~~~~~~~~^^^^^^^
  File "/Users/boyu/GitHubProjects/mesa-examples/gis/agents_and_networks/src/agent/building.py", line 40, in __hash__
    return hash(self.unique_id)
                ^^^^^^^^^^^^^^
AttributeError: 'Building' object has no attribute 'unique_id'

This PR should fix it. But I'm not very sure whether this will cause any trouble.

@wang-boyu wang-boyu added the bug Release notes label label Oct 15, 2024
Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔵 -1.0% [-1.9%, -0.2%] 🔵 +0.8% [+0.7%, +1.0%]
BoltzmannWealth large 🔵 +0.2% [-0.1%, +0.5%] 🔵 +3.5% [+2.8%, +4.2%]
Schelling small 🔵 +0.7% [+0.2%, +1.2%] 🔵 +0.4% [+0.2%, +0.6%]
Schelling large 🔵 +0.6% [-0.0%, +1.1%] 🔵 +0.5% [-1.7%, +2.9%]
WolfSheep small 🔵 -1.0% [-1.2%, -0.8%] 🔵 -0.1% [-0.3%, +0.0%]
WolfSheep large 🔵 +0.4% [-0.3%, +1.0%] 🔵 +2.4% [+1.6%, +3.5%]
BoidFlockers small 🔵 +2.1% [+1.7%, +2.7%] 🔵 +2.2% [+1.4%, +3.0%]
BoidFlockers large 🔵 +1.9% [+1.3%, +2.6%] 🔵 +2.0% [+1.4%, +2.5%]

@quaquel
Copy link
Member

quaquel commented Oct 16, 2024

This code change seems fine to me. There is no particular reason for the ordering in Agent.__init__ and the code also does not imply such ordering.

Why do you use a custom hash function in the example?

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.

Sounds smart to first assign all the variables to the agent, and then register with the model. Especially if it helps the hash problem!

@quaquel
Copy link
Member

quaquel commented Oct 16, 2024

Why do you use a custom hash function in the example?

Just to clarify, I am fine with the fix, but I think you don't need the hash function in your example anyway, avoiding the need for the fix. Hashing on custom classes is available by default already via id(self), which returns the memory address used by the object.

@wang-boyu
Copy link
Member Author

Initially there was only a __eq__() method so that I could do building_a == building_b instead of building_a.unique_id == building_b.unique_id. But having just the __eq__() method without __hash__() was causing troubles, and it was subsequently fixed in projectmesa/mesa-examples@f24626e as part of projectmesa/mesa-examples#187.

However now I looked at the example, building_a == building_b was not really used anywhere anymore, which was probably removed during development. So removing __eq__() as well as __hash__() from the example will most likely avoid the issue. But I still think that this fix is necessary in case someone else wants a custom __eq__() and __hash__().

Thanks @quaquel and @EwoutH for the review btw.

@quaquel
Copy link
Member

quaquel commented Oct 16, 2024

It's only if you want to to do a custom hash and base this on unique_id or any other attribute from super that you would get a problem. For __eq__, there is no problem.

But go ahead and merge this.

@wang-boyu
Copy link
Member Author

Merging now. Thanks!

@wang-boyu wang-boyu merged commit 9baee14 into projectmesa:main Oct 16, 2024
14 checks passed
@wang-boyu wang-boyu deleted the fix/register-agent-after-creating-id branch October 16, 2024 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants