-
Notifications
You must be signed in to change notification settings - Fork 874
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
Cacheable mesa #2194
base: main
Are you sure you want to change the base?
Cacheable mesa #2194
Conversation
for more information, see https://pre-commit.ci
Performance benchmarks:
|
|
This is some good work, well done! A couple thoughts for your consideration:
Again well done! Please change this to a draft and provide a comprehensive overview when you get the chance. Thank you! Finally, as no one can see what everyone can see, for the other maintainers, please feel free to correct or refine my advice or add your comments or thoughts. |
@rht I just realized if you click view file in the upper right you can see the |
@Chan-Dong-Jun this looks like an amazing and major effort! I agree with all of Tom's points, especially about communication.
Currently Mesa 3.0 is developed right here on the I want to give some examples of how a great PR message looks in our repo (in no particular order): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome you have included an example with it! Few points:
- It would be great if this was written "tutorial style", so with some explanations in between.
- Then you can also put it in https://github.com/projectmesa/mesa/tree/main/docs/tutorials
- Probably ensure that the outputs are not too long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what the motivation was for creating a whole new datacollector, and why it wasn't feasible to extend the current one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am aware that there are plans to revamp the entire datacollector. I thought that writing a wrapper class that has duplicates of some of the methods in the original classes would make the caching less dependent on the datacollector down the road.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds wel thought out, thanks!
I have already viewed on the browser. It still takes a while to scroll in order to see the important bits. |
|
2 main files (cacheable_model.py and cacheable_wrapper_example.ipynb), the rest are just my attempts