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: Initialize model _steps and _time during __new__ #2026

Merged
merged 1 commit into from
Feb 4, 2024

Conversation

rht
Copy link
Contributor

@rht rht commented Feb 4, 2024

The problem is that #1942 added _steps and _time during model __init__. _steps is used in the data collector and batch_run, so that both support a scheduler-less model implementation. #1942 broke backward compatibility because it requires super().__init__() inside user's model __init__.

This is to retain backward compatibility with Mesa 2.1.x behavior where a model super().__init__() is not necessary.

@rht
Copy link
Contributor Author

rht commented Feb 4, 2024

Copy link

github-actions bot commented Feb 4, 2024

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
Schelling small 🔵 +0.0% [-0.4%, +0.5%] 🔵 -0.1% [-0.3%, +0.2%]
Schelling large 🔵 +0.5% [-1.0%, +2.8%] 🔵 +4.4% [+0.4%, +9.2%]
WolfSheep small 🔵 +1.0% [+0.6%, +1.4%] 🔵 -0.2% [-0.4%, -0.0%]
WolfSheep large 🔵 +1.3% [+0.6%, +2.1%] 🔵 +3.8% [+1.7%, +5.8%]
BoidFlockers small 🔵 -1.2% [-1.8%, -0.6%] 🔵 +1.3% [+0.8%, +1.9%]
BoidFlockers large 🔵 -0.7% [-1.4%, +0.2%] 🔵 +0.8% [+0.4%, +1.3%]

Copy link
Contributor

@Corvince Corvince left a comment

Choose a reason for hiding this comment

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

Good, quick fix.

mesa/model.py Outdated Show resolved Hide resolved
This is to retain backward compatibility with Mesa 2.1.x behavior where
a model `super().__init__()` is not necessary.
@rht rht merged commit 967eada into projectmesa:main Feb 4, 2024
12 of 13 checks passed
@rht rht deleted the patch__steps branch February 4, 2024 09:26
@rht
Copy link
Contributor Author

rht commented Feb 4, 2024

cc: @EwoutH for patch release.

@EwoutH
Copy link
Member

EwoutH commented Feb 4, 2024

Will try to do tonight, and will make screen recording so others can do it as well.

Only this PR needs to be included right?

@EwoutH EwoutH added the bug Release notes label label Feb 4, 2024
@EwoutH
Copy link
Member

EwoutH commented Feb 4, 2024

Could someone write a 3-sentence summary with which problem this fixes for the 2.2.5 release notes?

@rht
Copy link
Contributor Author

rht commented Feb 4, 2024

The problem is that #1942 added _steps and _time during model __init__. _steps is used in the data collector and batch_run, so that both support a scheduler-less model implementation. #1942 broke backward compatibility because it requires super().__init__() inside user's model __init__. This PR is to have the backward compatibility back.

@rht
Copy link
Contributor Author

rht commented Feb 4, 2024

@EwoutH amended PR description. You may pick whichever version you prefer.

@rht
Copy link
Contributor Author

rht commented Feb 16, 2024

@EwoutH I know about doing the cherry picking and adding a tag for a release, but I'm not sure about the auto-draft for the release note. Still pending a release checklist (which has more automation than in the past) from your end.

@EwoutH
Copy link
Member

EwoutH commented Feb 16, 2024

Checklist is here: #1917 (comment)

Could you also convert https://github.com/projectmesa/mesa/blob/main/CONTRIBUTING.rst to markdown? Then I will add the checklist to it.

Go ahead of the patch release. Make sure to cherry pick from main to 2.2.x-maintenance. If you have any questions, hit me up! (Matrix is probably the fastest)

@rht
Copy link
Contributor Author

rht commented Feb 16, 2024

I looked at the checklist. Step 1 definitely can be scripted in a code as an automated check. Step 2 can be automated as well (API call with draft boolean). Step 3 can be input as a CLI flag. And so on.

@rht
Copy link
Contributor Author

rht commented Feb 16, 2024

I made the script for step 1 with the help of ChatGPT:

$ python fetch_unlabeled_prs.py
PR #2039: tests: Speed up test_batch_run - Merged at: 2024-02-16T12:58:38Z
PR #2029: [pre-commit.ci] pre-commit autoupdate - Merged at: 2024-02-05T22:57:38Z

Will update once I have automated the rest.

Comment on lines +60 to +62
# TODO: Remove these 2 lines just before Mesa 3.0
obj._steps = 0
obj._time = 0
Copy link
Member

Choose a reason for hiding this comment

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

Could you remind me why we need to remove these lines? Seems like the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because they are already specified in __init__. This was a hack to people who forget to super().__init__ their model.

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