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

More pythonic implementation of wolf sheep #2011

Merged
merged 2 commits into from
Jan 29, 2024
Merged

Conversation

quaquel
Copy link
Contributor

@quaquel quaquel commented Jan 27, 2024

This is a more Pythonic and single-file implementation of Wolf Sheep for benchmarking.

The single file is easier for profiling and line profiling.


from mesa import Model, Agent
from mesa import Agent, Model
Copy link
Contributor

Choose a reason for hiding this comment

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

These 3 lines can be condensed to be just import mesa, then calling them would be mesa.Agent, mesa.Model, mesa.space.MultiGrid, in line with np.array or pd.DataFrame. See https://github.com/projectmesa/mesa-examples/blob/479eaf389e1a98bda0bdba5aa8f24ede0d84e764/examples/wolf_sheep/wolf_sheep/model.py#L18.

Copy link
Contributor Author

@quaquel quaquel Jan 27, 2024

Choose a reason for hiding this comment

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

But that is slower.

To be clear, I don't care much either way, so if there is some mesa convention for this, I am fine changing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's about the same:

In [1]: import mesa

In [2]: m = mesa.Model()

In [3]: %timeit mesa.Agent(0, m)
627 ns ± 125 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [4]: from mesa import Agent, Model

In [5]: m2 = Model()

In [6]: %timeit Agent(0, m2)
627 ns ± 148 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

Copy link
Contributor

Choose a reason for hiding this comment

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

Except for the import lines, this PR LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really don't have strong opinions on this so if others want to change this as well, fine.

Note, however, that the Schelling model already uses the same style of importing as I have used here. I also personally prefer e.g., SchellingAgent(Agent) over SchellingAgent(mesa.Agent). This is really just a matter of taste.

Copy link
Contributor

Choose a reason for hiding this comment

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

The convention in the examples repo all uses mesa.x. The ones in the Agents.jl benchmark code are from the outdated version of the examples.

This is really just a matter of taste.

But in the case of Agents.jl benchmark, they count and compare the LOC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The convention in the examples repo all uses mesa.x.

I ment our benchmarks

But in the case of Agents.jl benchmark, they count and compare the LOC.

And we agree that this is only a rough proxy we should not obsess about.

Copy link
Contributor

Choose a reason for hiding this comment

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

I ment our benchmarks

The 3 versions of the code (mesa-examples, core Mesa benchmark, Agents.jl) ideally need to be kept in sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Once this is merged, I'll happily make PRs on the other libraries.

Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
Schelling small 🔵 +0.8% [+0.4%, +1.2%] 🔵 +0.0% [-0.1%, +0.2%]
Schelling large 🔵 +1.3% [+0.7%, +1.8%] 🔵 +0.9% [-1.8%, +4.0%]
WolfSheep small 🔵 -2.1% [-2.9%, -1.4%] 🟢 -6.1% [-7.2%, -5.0%]
WolfSheep large 🔵 +3.4% [-10.9%, +22.1%] 🟢 -13.0% [-15.3%, -10.7%]
BoidFlockers small 🔵 +0.9% [+0.4%, +1.4%] 🔵 +0.5% [-0.2%, +1.3%]
BoidFlockers large 🔵 +0.2% [-0.4%, +0.8%] 🔵 +0.2% [-0.1%, +0.6%]

@EwoutH
Copy link
Contributor

EwoutH commented Jan 27, 2024

Thanks! Would it be possible to split into two commits: One that makes it a single file, and then the actual changes? This diff is very difficult to review.

@quaquel
Copy link
Contributor Author

quaquel commented Jan 27, 2024

There is no real point looking at the diff here. It's just a clean reimplementation.

@tpike3
Copy link
Contributor

tpike3 commented Jan 29, 2024

As the build is failing at SugarScape_g1mt, I am going to merge.

@tpike3 tpike3 merged commit 2dc485f into projectmesa:main Jan 29, 2024
12 of 13 checks passed
@EwoutH EwoutH added docs Release notes label Performance enhancement Release notes label and removed enhancement Release notes label labels Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Release notes label Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants