-
Notifications
You must be signed in to change notification settings - Fork 180
Integrate Hotelling Law Extension into Mesa Examples #120
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
Conversation
Thanks a lot for the PR! Looks like an interesting model to have in our example library. I requested two of our maintainers to review it. |
Thanks a lot @EwoutH |
) | ||
new_position = self.random.choice(possible_steps) | ||
self.model.grid.move_agent(self, new_position) | ||
elif self.model.environment_type == "line": |
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.
You could simplify the movement code by using the branch for "grid"
(above), and for line environment, you simply initialize the grid with a width of 1.
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.
@rht Please help me understand and elaborate this
You could simplify the movement code by using the branch for "grid" (above)
you simply initialize the grid with a width of 1.
It's only visualization, just wanted to show the single in center, added that way, adding width 1 it's displaying towards left with remaining grid visible, i'm not sure how it works
Need your help in explaining this
Thank you so much for the review @rht
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.
If your concern is the centering of the visualization of the grid, it shouldn't complicate the core logic implementation. I recommend using the newer experimental visualization, https://mesa.readthedocs.io/en/stable/tutorials/visualization_tutorial.html (the current visualization you are using will be deprecated in Mesa 3.0). By default, you will get something like
This is because the size auto-adjust algorithm doesn't cover extreme case when width << height yet.
But if you pass in a specific size, e.g. 10 to the agent portrayal, you will get
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'm going through visualization with Jupyter - Does that mean visualization can only work with the Jupyter environment, and currently built-in server visualization will be deprecated? no standalone execution?
So models should be jupyter notebooks going forward...?
I'm a bit confused with API documentation and comments. I believe the current stable version of API is 2.2.4, which is what we have used in the project. Does this mean that 3.0 is an overhaul of the complete API?
Please advise if there is any document, detailing 2.2.4 vs 3.0 api, mean while I'm going through project code on mesa
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.
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.
@rht please help advise
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.
All the controls in the old viz have been implemented in the new viz. See https://github.com/projectmesa/mesa/blob/4cc03a4300ce2f0600efa3ef45a61f16d2290dc0/mesa/experimental/jupyter_viz.py#L365-L371 for the checkbox. You specify as
"layout": {"type": "Select", "value": "grid", "values": ["grid", "line"]}
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.
@rht thank you. I appreciate the patience explaining to me the required details
I've added new UI. Kindly review
# Function to compute the average price of all store agents in the model. | ||
def compute_average_price(model): | ||
"""Compute the average price of all stores.""" | ||
store_prices = [agent.price for agent in model.schedule.agents] |
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.
model.agents
is the new API, and is simpler.
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.
The whole function can be simplified to return np.mean(model.agents.get("price"))
.
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.
The intermediate variables are not necessary, because the docstring is already sufficiently descriptive.
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.
fixed these comments
If you could add a test that check's for the prediction of the Hotelling's law, e.g. mesa-examples/examples/sugarscape_g1mt/tests.py Lines 19 to 33 in 1933cbc
pytest tests.py . The functions with prefix of test_* are automatically run by pytest . Also, in the visualization, you are meant to show that the standard deviation of the price is decreasing over time.
|
-fixing review comments 1. The self.can_move check is redundant here, because it's already done in self.move 2. model.agents is the new API, and is simpler. 3. The whole function can be simplified to return np.mean(model.agents.get("price")). 4. The intermediate variables are not necessary, because the docstring is already sufficiently descriptive. 5. added tests for slope
for more information, see https://pre-commit.ci
Added tests.py |
for more information, see https://pre-commit.ci
@aumashankar and @rht, thanks for the hard work in the past days. I'm curious, how far along the review process are we? Is there anything I can do to help? |
@EwoutH we've used old UI and some old api's with agent schedule @rht directed me to new mesa3.0 UI, helping review it in the process. last commit includes the feedback changes from @rht which can further help with review if it's good |
examples/hotelling_law/Readme.md
Outdated
```bash | ||
python run.py | ||
|
||
solara run app.py (mesa 3.0) |
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.
People might misinterpret (mesa 3.0)
as something to be typed. Better omit it.
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.
updated
examples/hotelling_law/app.py
Outdated
import solara | ||
from mesa.experimental import JupyterViz | ||
|
||
from .model import HotellingModel |
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.
The relative import caused this error message on my machine (Python 3.11)
ImportError: attempted relative import with no known parent package
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.
You can remove the relative imports for now.
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.
Adopted project structure removed relative imports
I ran the Solara simulation, and observed the variance going up over time instead of going down. Doesn't this contradict Hotelling's law? |
This observation is due to Initial Conditions and Randomness for the basic model Observing an increase in price variance over time in the simulation does not necessarily contradict Hotelling's Law. as it depends on factors. We are working further with consumer preferences and market share and no of consumers (might be another PR) where Hotelling's Law, in the context of spatial competition, suggests that businesses will choose to locate themselves in a manner that minimizes competition, Edit: changed test as variance can increase or decrease depending on factors. |
…ts and changed test for variance as for hotelling law variance need not decrease depends on factors
for more information, see https://pre-commit.ci
The basic model itself needs to display the Hotelling law, because it is what the example is named after. At least, try changing the default params until the variance reduction happens, and in the readme, tell the reader about the param range, and when and why it starts breaking down, or that the timescale could be longer. This is standard practice for sensitivity analysis. If you want to speed up the simulation step duration, you can tweak the param |
There are multiple parts to it, one aspect is that, hotelling law in itself doesn't contradict as it depends on other parameters. However, I'll get back in a day or two to integrate those here. |
…pricing and location. Removed old implementation
for more information, see https://pre-commit.ci
@rht updated PR with consumer preferences, market share, visualizations and tests for simulation kindly review. I appreciate your patience and helping us with details (we are learning the framework) |
examples/hotelling_law/Readme.md
Outdated
- matplotlib | ||
- numpy | ||
- scipy | ||
- jupyter |
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.
jupyter
is not needed. Everything else is auto-installed when installing mesa
, except for scipy
.
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.
updated requirements.txt and Readme.md
Thanks a lot for the review @rht
Everything else LGTM. |
Thank you |
git+https://github.com/projectmesa/mesa-examples |
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.
This line is not needed, and is confusing to be here.
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.
ah.. I was just going through other samples... I'm removing it... can you explain the context how other examples using it
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.
updated
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.
They should be removed. The experimental visualization used to live in the mesa-examples repo.
@@ -0,0 +1,34 @@ | |||
import cProfile |
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.
Forgot to comment this: the file cprofile_test.py shouldn't be part of the example.
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.
ok. removing this file
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.
remvoed
Merged, thank you @aumashankar. |
@rht Thanks a lot for the review and all the help provided in learning the mesa framework. (the old and new) |
@aumashankar Thank you for YOUR contribution and for making requested changes. We appreciate your diligence and contribution! |
Closes issue #119: Hotelling Law Extension using Mesa (NetLego example using mesa)
Online demo: http://178.128.112.142:8521/