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

Minor edits to benchmarking code #1985

Merged
merged 18 commits into from
Jan 21, 2024
Merged

Minor edits to benchmarking code #1985

merged 18 commits into from
Jan 21, 2024

Conversation

quaquel
Copy link
Member

@quaquel quaquel commented Jan 21, 2024

@EwoutH
Copy link
Member

EwoutH commented Jan 21, 2024

Let's see if the CI is working: /rerun-benchmarks

.gitignore Outdated Show resolved Hide resolved
Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
SchellingModel small 🔵 -2.8% [-3.2%, -2.4%] 🔵 -0.1% [-1.1%, +0.7%]
SchellingModel large 🔵 -2.7% [-3.2%, -2.2%] 🔴 +5.4% [+3.8%, +7.0%]
WolfSheep small 🔵 -0.4% [-1.0%, +0.1%] 🔵 +0.6% [+0.3%, +0.9%]
WolfSheep large 🟢 -27.9% [-42.8%, -13.4%] 🔵 +1.2% [-1.9%, +4.4%]
BoidFlockers small 🔵 -0.9% [-1.2%, -0.5%] 🔵 -0.5% [-1.3%, +0.2%]
BoidFlockers large 🔵 -0.1% [-0.7%, +0.6%] 🔵 -0.5% [-1.1%, +0.1%]

@EwoutH
Copy link
Member

EwoutH commented Jan 21, 2024

Let me know when this is ready for review

commit c0de4a1
Author: Ewout ter Hoeven <E.M.terHoeven@student.tudelft.nl>
Date:   Sun Jan 21 14:43:08 2024 +0100

    Add CI workflow for performance benchmarks (projectmesa#1983)

    This commit introduces a new GitHub Actions workflow for performance benchmarking. The workflow is triggered on `pull_request_target` events and can be triggered with a "/rerun-benchmarks" comment in the PR. It should be compatible with PRs from both forks and the main repository. It includes steps for setting up the environment, checking out the PR and main branches, installing dependencies, and running benchmarks on both branches. The results are then compared, encoded, and posted as a comment on the PR.
@quaquel
Copy link
Member Author

quaquel commented Jan 21, 2024

Let me know when this is ready for review

It should be alright now. Please squash and merge this because I made a bit of a mess of the commit history.

@EwoutH
Copy link
Member

EwoutH commented Jan 21, 2024

Could you update the PR message with a bit of motivation for the changes made?

/rerun-benchmarks

@quaquel
Copy link
Member Author

quaquel commented Jan 21, 2024

Could you update the PR message with a bit of motivation for the changes made?

/rerun-benchmarks

I already did so, or do you want to know more? But if so, what?

Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
SchellingModel small 🔵 -2.4% [-2.8%, -2.0%] 🔵 -0.3% [-0.9%, +0.2%]
SchellingModel large 🔵 -1.9% [-2.5%, -1.3%] 🔵 -0.0% [-0.7%, +0.7%]
WolfSheep small 🔵 -0.0% [-0.3%, +0.2%] 🔵 +0.0% [-0.1%, +0.1%]
WolfSheep large 🔵 -4.6% [-13.7%, +0.6%] 🔵 +0.7% [+0.2%, +1.3%]
BoidFlockers small 🔴 +3.6% [+3.3%, +4.0%] 🔵 +0.4% [-0.2%, +0.9%]
BoidFlockers large 🔵 +2.9% [+2.5%, +3.3%] 🔵 +0.3% [-0.2%, +0.8%]

Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
SchellingModel small 🔵 -1.0% [-1.5%, -0.5%] 🟢 -5.7% [-6.4%, -4.9%]
SchellingModel large 🔵 -1.2% [-2.0%, -0.2%] 🟢 -5.8% [-6.4%, -5.1%]
WolfSheep small 🔵 +1.3% [+0.9%, +1.6%] 🔵 -1.2% [-1.3%, -1.0%]
WolfSheep large 🟢 -21.0% [-37.0%, -5.3%] 🔵 +2.9% [-3.1%, +9.2%]
BoidFlockers small 🔴 +3.8% [+3.2%, +4.4%] 🔵 +1.2% [+0.6%, +1.8%]
BoidFlockers large 🔵 +3.1% [+2.0%, +3.8%] 🔵 +0.8% [+0.0%, +1.6%]

@EwoutH EwoutH requested a review from rht January 21, 2024 21:11
@EwoutH EwoutH requested a review from Corvince January 21, 2024 21:11
@EwoutH
Copy link
Member

EwoutH commented Jan 21, 2024

LGTM, thanks for this effort. @rht or @Corvince could one of you do the final review and squash merge if ok?

Did you also observe the Schelling being faster on your system? Or is that an anomaly?

In the long run we should try to keep these models in sync with ABMFrameworksComparison. Do we want to make further improvements on these models in the short run?

@rht
Copy link
Contributor

rht commented Jan 21, 2024

In the long run we should try to keep these models in sync with ABMFrameworksComparison. Do we want to make further improvements on these models in the short run?

The sync with Agents.jl devs needs to happen soon, if possible. I think they would appreciate to know if we are now monitoring the performance as well.

@rht
Copy link
Contributor

rht commented Jan 21, 2024

Did you also observe the Schelling being faster on your system? Or is that an anomaly?

Haven't tested, but the finding is probably not a blocker to this PR being merged.

@rht rht merged commit 4d0eeec into projectmesa:main Jan 21, 2024
12 of 13 checks passed
@quaquel
Copy link
Member Author

quaquel commented Jan 22, 2024

LGTM, thanks for this effort. @rht or @Corvince could one of you do the final review and squash merge if ok?

Did you also observe the Schelling being faster on your system? Or is that an anomaly?

In the long run we should try to keep these models in sync with ABMFrameworksComparison. Do we want to make further improvements on these models in the short run?

  1. I think there is still quite some noise in the benchmarks. But one issue with the old schelling code was that it did not use self.random properly in all places. This probably contributes to even more noise.
  2. In my view we should indeed keep our versions in sync with ABMFrameworksComparison. It would be good to push any changes to them in the context of a new stable release of MESA.

I also noticed that, in particular, WolfSheep is not a good Python implementation. There is a lot of unnecessary code duplication going on. I have the impression that some implementations were done to mimic NetLogo's version. Which is fine for a tutorial for people coming from NetLogo. However, I would argue that the benchmark (and most examples) should reflect the Pythonic and Mesa way of doing things. I am happy to push a WolfSheep reimplementation if desired (But also see https://github.com/quaquel/mesa/blob/cell-space/benchmarks/WolfSheep/wolf_sheep.py, for the direction in which I would change it.

@EwoutH
Copy link
Member

EwoutH commented Jan 22, 2024

Sounds good, would you like to do both (pushing to ABMFrameworksComparison and speeding up WolfSheep yourself?

@quaquel
Copy link
Member Author

quaquel commented Jan 22, 2024

Yes, I'm fine with doing both (although it is more about cleaning up wolfsheep code; I have no idea about performance, but it is probably small).

@rht
Copy link
Contributor

rht commented Jan 24, 2024

I think the first step is to simplify the WolfSheep at https://github.com/projectmesa/mesa-examples, then next all the other WolfSheep's (in Mesa's benchmark and Agents.jl's benchmark) should be updated to be almost the same as this reference implementation.

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

Successfully merging this pull request may close these issues.

3 participants