-
Notifications
You must be signed in to change notification settings - Fork 879
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
docs: Update the intro tutorial with extended analysis #1625
Conversation
Update the intro tutorial. Use Seaborn for more flexible and easier to read and manipulate plots. Add parts about analysing agent reporters and showing some different kinds of plots. - Also make seaborn a required dependency - Also let the DataCollector save index name(s) for the DataFrames returned. This enables not manually having to label the x-axis each time when the time step is used on the x-axis, and improved unstacking.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## doc_builds #1625 +/- ##
==============================================
+ Coverage 81.81% 81.84% +0.02%
==============================================
Files 18 18
Lines 1386 1388 +2
Branches 271 271
==============================================
+ Hits 1134 1136 +2
Misses 206 206
Partials 46 46
☔ View full report in Codecov by Sentry. |
@jackiekazil, @tpike3, @rht, @Tortar and others, if you could find the time this week (or weekend) to review the changes to the tutorial that would be awesome. Now it’s still fresh in my head so making changes is relatively fast and easy. |
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.
@EwoutH Can you please clear your outputs in your kernel? Otherwise there is a significant numbers of changes that are not substantive.
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.
Yes, I can definitely do that if preferred. It might be a bit harder to see the results from the changes however.
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.
Are your running the make html
command so it updates the read the docs/ sphinx for the tutorial? This way you tutorial changes will be visible as part of the tutorial.
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.
Not yet. I have no experience with make html
, for converting Jupyter notebooks I normally use nbconvert.
The current format is .rst
, does make html
convert to that format (ReStructuredText) by default? Where/how do I run 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.
So make html
is part of Sphinx which will autobuilds the docs. If you don't want go through that process, no worries, just make changes to the .ipynb
(plese clear your cell outputs) and then when I am updating the docs as part of 2.0 I will take care of it (but it may be a few weeks before it shows up).
@@ -8,7 +8,7 @@ | |||
|
|||
from setuptools import find_packages, setup | |||
|
|||
requires = ["click", "cookiecutter", "networkx", "numpy", "pandas", "tornado", "tqdm"] | |||
requires = ["click", "cookiecutter", "networkx", "numpy", "pandas", "tornado", "tqdm", "seaborn"] |
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 will raise this at the dev meeting today, but generally, adding dependencies is a considered a very big deal as we try and keep Mesa as lean as possible to reduce complexity and dependency issues.
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.
Yes, I understand. Since matplotlib itself also isn't an dependency, we can also introduce an optional "plotting" dependency which includes matplotlib and seaborn.
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 dev meeting got pushed to the 15th, however, instead of adding to ore dependencies can you add to the requirements.txt
? Please see comments in the .ipynb
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.
The requirements.txt
of Mesa itself or of the boltzman wealth model in 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.
Of the Boltzman wealth please this way we keep Mesa lean. So matplotlib and seaborn will just be a dependency for the tutorial and not for 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.
Yeah, Matplotlib/Seaborn shouldn't be a required dependency to install Mesa. I think the ModuleNotFoundError
is a sufficient error message, that we shouldn't have a requirements.txt that is specific to the tutorial.
mesa/datacollection.py
Outdated
@@ -244,6 +246,7 @@ def get_agent_vars_dataframe(self): | |||
columns=["Step", "AgentID", *rep_names], | |||
index=["Step", "AgentID"], | |||
) | |||
df.index.names = ["Step", "AgentID"] |
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.
Why do you need to add line 249; line 247 makes the index "Step" and "AgentID"
return pd.DataFrame(self.model_vars) | ||
df = pd.DataFrame.from_dict(self.model_vars) | ||
df.index.name = "Step" | ||
return df |
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.
Two comments/questions:
-
This change would explicitly show that the index is the "Step"? I don't have any issues with this change as it is consistent with principle of least surprise and makes it constant with agent dataframe. But could you also please update docstrings as this would make it no longer implicit.
-
pd.DataFrame(self.model_vars)
andpd.DataFrame.from_dict(self.model_vars)
is there a particular reason for changing it?
"```\n", | ||
"\n", | ||
"When you do that, it will install Mesa itself, as well as any dependencies that aren't in your setup yet. Additional dependencies required by this tutorial can be found in the **examples/boltzmann_wealth_model/requirements.txt** file, which can be installed directly form the github repository by running:\n", | ||
"\n", | ||
"```bash\n", | ||
" $ pip install -r https://raw.githubusercontent.com/projectmesa/mesa/main/examples/boltzmann_wealth_model/requirements.txt\n", | ||
" $ pip install --upgrade -r https://raw.githubusercontent.com/projectmesa/mesa/main/examples/boltzmann_wealth_model/requirements.txt\n", | ||
"```\n", |
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.
@EwoutH this link is broken since we moved the examples to the mesa-examples repo.
Could you fix the boltzman wealth link and instead of adding in seaborn to the Mesa dependencies why not just add seaborn to the requirements.txt file? (Although we need to update this a pipfile) but that can be later on down the road and should not hold up this merge.
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.
Good catch, done!
@EwoutH thanks for doing and sorry my comments have been a little chaotic. I have had some work deadlines have had to dig in piecemeal. |
- Update the dependency installation link in the tutorial to https://github.com/projectmesa/mesa-examples/blob/main/examples/boltzmann_wealth_model_network/requirements.txt - Update the DataCollector to remove redundant changes (adding index names and the to_dict().
The plots definitely look more closer to ggplot2 plots. The DataFrames are optional in some of them, and there is virtually no added complication either. I haven't had the time to review the whole intro_tutorial.ipynb content because the diff is huge, with some sections being rewritten, and some being added. |
@EwoutH we have some tutorial changes at PyCon US sprints. Changes will probably slow down after this week. Most of the changes at PyCon are textual in nature -- basically writing a smoother & more welcoming tutorial. |
Thanks for getting back. Do you need anything from me at this moment? What's now the next step? |
"source": [ | ||
"Lastly, we want to take a look at the development of the Gini coefficient over the course of one iteration. Filtering and printing looks almost the same as above, only this time we choose a different episode." | ||
] | ||
"In this case it looks like the Gini coefficient increases slower for smaller populations. This can be because of different things, either because the Gini coefficient is a measure of inequality and the smaller the population, the more likely it is that the agents are all in the same wealth class, or because there are less interactions between agents in smaller populations, which means that the wealth of an agent is less likely to change." |
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.
👍 for the analysis.
@EwoutH do you mind updating intro_tutorial.rst with the changes? It's easier to see the diff there. |
This will be another major effort to rebase.
This is now over a month ago. This PR got closed without a clear motivation. I find it for some reason really difficult to contribute to Mesa, and it really demotivates me to make the effort. And that sucks, because I think we need to have a good ABM package in an accessible programming language. |
You don't have to rebase. I can handle that later. What do you think of transitioning the tutorial to mainly .rst files + image files? This way, no Jupyter notebooks with heavy diffs, no computational states recorded, and the plots are separated from the texts. If this is more practical? |
I agree with this statement. |
I have been chewing this for a couple days and have few good answers. I will put this as the top priority for the next dev meeting, but to excel in the Open Source we need to figure this out. I will also do some research on best habits so I come with proposals. |
With #1694 ( |
I reopened this PR as #1716. |
Update the intro tutorial. Use Seaborn for more flexible and easier to read and manipulate plots. Add parts about analysing agent reporters and showing some different kinds of plots.
For now I only updated the Jupyter Notebook, I will convert to .rst and update all the images once everybody agrees on the updated context.
I was also thinking about an more extensive experimental and analysis tutorial with a more complex model, but that's for another time.