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

Remove schedulers from benchmark models. #2308

Merged
merged 3 commits into from
Sep 21, 2024
Merged

Conversation

quaquel
Copy link
Member

@quaquel quaquel commented Sep 21, 2024

this removes the schedulers from the benchmark models (in one case it was in the init but not used in step anyway).

@quaquel quaquel requested a review from EwoutH September 21, 2024 08:34
Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔴 +20.3% [+18.9%, +21.7%] 🔴 +23.7% [+23.4%, +23.9%]
BoltzmannWealth large 🔴 +28.6% [+28.1%, +29.1%] 🔴 +22.0% [+21.1%, +22.8%]
Schelling small 🟢 -8.0% [-8.4%, -7.6%] 🔵 -1.0% [-1.2%, -0.8%]
Schelling large 🟢 -9.0% [-9.4%, -8.5%] 🔵 -1.7% [-2.2%, -1.2%]
WolfSheep small 🔵 -0.7% [-0.9%, -0.5%] 🔵 -0.0% [-0.2%, +0.1%]
WolfSheep large 🔵 -2.9% [-3.8%, -2.0%] 🔵 -2.2% [-4.8%, -0.2%]
BoidFlockers small 🟢 -8.0% [-8.8%, -7.4%] 🔵 +0.9% [+0.2%, +1.6%]
BoidFlockers large 🟢 -8.4% [-8.9%, -8.0%] 🔵 -0.3% [-1.2%, +0.7%]

@quaquel quaquel added trigger-benchmarks Special label that triggers the benchmarking CI and removed trigger-benchmarks Special label that triggers the benchmarking CI labels Sep 21, 2024
Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔴 +23.1% [+21.9%, +24.3%] 🔴 +24.5% [+24.3%, +24.7%]
BoltzmannWealth large 🔴 +29.4% [+29.0%, +29.7%] 🔴 +23.9% [+23.3%, +24.7%]
Schelling small 🟢 -8.0% [-8.4%, -7.6%] 🔵 -1.2% [-1.4%, -1.0%]
Schelling large 🟢 -8.7% [-8.9%, -8.5%] 🔵 -1.1% [-2.1%, -0.2%]
WolfSheep small 🔵 +0.1% [-0.1%, +0.2%] 🔵 +0.0% [-0.2%, +0.2%]
WolfSheep large 🔵 -1.1% [-1.4%, -0.7%] 🔵 +0.3% [-0.2%, +0.7%]
BoidFlockers small 🟢 -8.0% [-8.5%, -7.4%] 🔵 -0.4% [-1.0%, +0.2%]
BoidFlockers large 🟢 -8.3% [-8.7%, -7.9%] 🔵 -1.7% [-2.1%, -1.1%]

@EwoutH
Copy link
Member

EwoutH commented Sep 21, 2024

I don’t understand how BoltzmannWealth gets slower while doing less (being already on shuffle_do()).

Probably just a fluke.

@EwoutH EwoutH added maintenance Release notes label ignore-for-release PRs that aren't included in the release notes and removed ignore-for-release PRs that aren't included in the release notes labels Sep 21, 2024
@quaquel quaquel added trigger-benchmarks Special label that triggers the benchmarking CI and removed trigger-benchmarks Special label that triggers the benchmarking CI labels Sep 21, 2024
Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔴 +28.7% [+27.0%, +30.2%] 🔴 +25.4% [+25.2%, +25.6%]
BoltzmannWealth large 🔴 +30.0% [+29.6%, +30.3%] 🔴 +33.5% [+32.7%, +34.3%]
Schelling small 🟢 -8.0% [-8.3%, -7.6%] 🔵 -0.1% [-0.3%, +0.1%]
Schelling large 🟢 -7.6% [-8.3%, -6.9%] 🔴 +9.7% [+7.8%, +11.8%]
WolfSheep small 🔵 +0.5% [+0.2%, +0.9%] 🔵 +1.8% [+1.6%, +2.0%]
WolfSheep large 🔵 +2.1% [+0.1%, +4.1%] 🔵 +7.4% [+2.6%, +12.3%]
BoidFlockers small 🟢 -7.8% [-8.4%, -7.1%] 🔵 +1.9% [+1.0%, +2.7%]
BoidFlockers large 🟢 -9.0% [-9.2%, -8.7%] 🔵 +0.8% [+0.4%, +1.3%]

@quaquel
Copy link
Member Author

quaquel commented Sep 21, 2024

I think it has to do with the data collection in the Boltzman wealth model. I need to dig deeper to confirm.

@quaquel
Copy link
Member Author

quaquel commented Sep 21, 2024

My hunch is correct. The old boltzman wealth model contained a bug. It used a schedule but no agents were added to it. The datacollecter then collected data from the agents in the schedule (which were None). So, the new benchmark is correct.

Also, this means that the deprecation of schedules should result in an update to the datacollector. In fact, I think the entire agent_collector can be deprecated in favor of the new agent type stuff.

@EwoutH
Copy link
Member

EwoutH commented Sep 21, 2024

Yeah, that's correct, you can also see it in #2300. These two are completely equivalent:

 self.datacollector = DataCollector(
     agent_reporters={"energy": "energy"},
     agenttype_reporters={
         Agent: {"energy": "energy"}
     }

We will redesign this whole thing any way. Let's leave the API alone for now.

Implementation wise, these parts could be updated as soon as the schedulers are actually removed.

mesa/mesa/datacollection.py

Lines 231 to 237 in e6874ad

agent_records = map(
get_reports,
model.schedule.agents
if hasattr(model, "schedule") and model.schedule is not None
else model.agents,
)
return agent_records

In between we could add a warning. But I don't think that's easy or worth it.

This PR is good to go!

@quaquel quaquel merged commit 48c27e3 into projectmesa:main Sep 21, 2024
11 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants