Skip to content

Model: Replace get_agents_of_type method with agents_by_type property #2267

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 5 commits into from
Sep 3, 2024

Conversation

EwoutH
Copy link
Member

@EwoutH EwoutH commented Sep 2, 2024

This PR replaces the Model method get_agents_of_type() with an agents_by_type property, which directly returns the dict.

Instead of using:

model.get_agents_of_type(Sheep)

You should now use:

model.agents_by_type[Sheep]

Since we also use agents and not get_agents, it's more intuitive to directly have access to the object itself (in this case the agents_by_type dict). It's also is more concise and since you have full dict access, more flexible.

Examples and tests are updated and an deprecation warning is added for get_agents_of_type(). Example models are updated in:

We also use `agents` and not `get_agents`, so `agents_of_type` is more consise and consistent.
@EwoutH EwoutH added enhancement Release notes label deprecation When a new deprecation is introduced labels Sep 2, 2024
Copy link

github-actions bot commented Sep 2, 2024

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔵 -0.6% [-1.9%, +0.6%] 🔵 -0.6% [-0.8%, -0.5%]
BoltzmannWealth large 🔵 -0.9% [-32.3%, +35.8%] 🔵 -0.7% [-2.9%, +1.5%]
Schelling small 🔵 +0.1% [-0.3%, +0.4%] 🔵 +1.4% [+1.0%, +1.7%]
Schelling large 🔵 +0.6% [-0.4%, +1.4%] 🔵 +3.5% [+2.1%, +4.9%]
WolfSheep small 🔵 -2.0% [-3.1%, -0.8%] 🔵 -2.5% [-2.9%, -2.2%]
WolfSheep large 🔵 -1.7% [-2.2%, -1.1%] 🟢 -6.2% [-7.1%, -5.4%]
BoidFlockers small 🔵 -1.6% [-2.0%, -1.1%] 🔵 -0.5% [-1.2%, +0.2%]
BoidFlockers large 🔵 -2.3% [-3.0%, -1.6%] 🔵 -0.9% [-1.9%, +0.1%]

@quaquel
Copy link
Member

quaquel commented Sep 2, 2024

I am not against making this change, but agents is an attribute while get_agents_of_type is a method. You are thus comparing two different things. What about turning agents_of_type into an attribute as well, so you would do model.agents_of_type[Sheep]. I prefer methods with names that clearly indicate that something is done (hence the use of get).

@EwoutH
Copy link
Member Author

EwoutH commented Sep 2, 2024

Fair point. At this point we can just expose the agents_by_type dict though a property, right?

@EwoutH
Copy link
Member Author

EwoutH commented Sep 2, 2024

Implemented as suggested!

@quaquel
Copy link
Member

quaquel commented Sep 2, 2024

If you add a test for this in test_model, its good to go as far as I am concerned.

@EwoutH EwoutH changed the title model: Rename get_agents_of_type to agents_of_type Model: Replace get_agents_of_type method with agents_by_type property Sep 3, 2024
@EwoutH
Copy link
Member Author

EwoutH commented Sep 3, 2024

Tests are added and the PR description and title are updated with some explanation and examples.

Thanks for reviewing!

@EwoutH EwoutH merged commit 221084d into projectmesa:main Sep 3, 2024
9 of 12 checks passed
EwoutH added a commit to EwoutH/mesa that referenced this pull request Sep 24, 2024
…erty (projectmesa#2267)

This PR replaces the Model method `get_agents_of_type()` with an `agents_by_type` property, which directly returns the dict.

Instead of using:
```Python
model.get_agents_of_type(Sheep)
```
You should now use:
```Python
model.agents_of_type[Sheep]
```
Since we also use `agents` and not `get_agents`, it's more intuitive to directly have access to the object itself (in this case the `agents_by_type` dict). It's also is more concise and since you have full dict access, more flexible.

Examples and tests are updated and an deprecation warning is added for `get_agents_of_type()`.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
deprecation When a new deprecation is introduced enhancement Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants