-
Notifications
You must be signed in to change notification settings - Fork 961
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
Requested Edits from JOSS #2666
Comments
I am going to do a sweep of the smaller comments -- probably this weekend. |
@jofmi thanks a lot for your detailed and thorough review! It’s really nice to have a fresh sets of eyes on it. Jackie thanks for creating the tracking issue. At some point we had experimental split out in a separate section, maybe we could revert to that. Also, we’re stabilizing the cell space so that shouldn’t be an issue. I see a few options for the other features: Curious what everybody (including @jofmi) thinks about both. As on the other feedback (please check if you can work on it):
|
@jofmi @martibosch I invited you both as "Triager" of the Mesa repo, so you can directly review PRs related to the JOSS paper. @projectmesa/maintainers please let me know if you can pick up the points above. @wang-boyu if you see anything you could pick up, also please let me know. |
@wang-boyu, for the stuff that was merged in #2678, can you mark it off in the list above, so we know it is complete? |
Done! |
@projectmesa/maintainers I updated the first post in this issue with the new comments from Martí Bosch, our second reviewer. Those have the prefix [from Marti]. Now, let's try to distribute some task. Please mark your checkbox if you're okay with picking this up, and if not or if you have an other idea or thing you want to do, let us know.
@quaquel I know you are very busy right now, but maybe you could review some of the incoming PRs on the paper. @martibosch, I temporarily invited you to become a Triager in Mesa, please consider accepting so you can also review these PRs. This will only be for the duration of the paper review. @Sahil-Chhoker if you want to contribute anything to our paper as collaborator, I'm good with that. We can mention you in the acknowledgements, if that matters to you. Let's wrap it up! |
The line numbers are quite helpful in the comments, but with all the edits -- I am struggling to track down which line. Does anyone know how to refer to a previous version of the paper? Example of what I am struggling to identify specifically (but this probably won't be the only one) -- |
@jackiekazil would you want to go back in de PDF or Markdown version?
Thanks for the PR! |
@EwoutH It looks like the links to the papers -- link to the same paper. So the links do no update but the paper is updated when it is rebuilt. Trying to figure out another way without digging into git history, but maybe that is it. Can you do a sanity check for me? |
No it's just a version from git history I'm afraid. Our CI does build a PDF on every change, so for every commit there should be a PDF available in our CI here. |
A review with requested edits have come in from JOSS. The checklist is below. Let's use this ticket for tracking.
References:
Major comments
Comments on the documentation (@EwoutH)
On the index page, I would add a sentence explaining what the "rec" and "all" extensions are needed for respectively (e.g. the visualization package?). docs: Clarify recommended dependencies in installation guide #2672
I would suggest to put the "Overview of the MESA library" section within "Getting started" as a seperate page, as it is a handy reference that is more accessible than the API documentation, also after getting started. docs: Split off the overview page, extend with spaces/activation #2673
In addition, it would be great for the overview to cover all features of MESA. Compared to the paper, spaces and time advancement are missing here. docs: Split off the overview page, extend with spaces/activation #2673
[from Marti] In the "Mesa core examples" section, there's confusion between referring to the mesa-examples repository while code snippets correspond to files in https://github.com/projectmesa/mesa/tree/main/mesa/examples. It would be better to put everything in one place and mention in the documentation that users can clone it and go to each example's folder instead of having them copy each snippet in a separate file. [JOSS] Docs: Clarify difference between core and user examples #2706
[from Marti] It may be a good idea to explicitly mention the
number_processes
argument ofmesa.batch_run
in the Batchrunner API page. The documentation should also explain why it's "strongly advised to only run in parallel using a normal python file (so don't try to do it in a jupyter notebook)". [JOSS] docs: Improve batch_run documentation for parallel processing #2707Comments on the introductory tutorial (@tpike3)
In the introductory tutorial, it is a bit confusing what is supposed to go in the jupyter or the python file. For simplification of the tutorial, I would recommend for the whole tutorial to be within the jupyter file. If you want to keep the recommendation to split up files, I would add it as a tipp in "Best practices".
For the setup, using code cells with terminal commands "!pip install --upgrade mesa[rec]" (note the exclamation mark at the start) enables installation within the jupyter file, removing the need for special instructions for google collab.
Please put line breaks into the comments that are longer than 79 characters, allowing the documentation do be read on half-screen.
Some sentences do not start capitalized.
[from Marti] In the "Agent management" section, the AgentSet class is mentioned but doesn't appear explicitly in the code. It should be clarified that it's instantiated automatically with the models and then made available through the agents attribute. This applies to both the paper and the introductory tutorial.
[from Marti] The introductory and visualization tutorials would be better served as self-contained notebooks with a "pip install" cell to install the tutorial's requirements. For instance, the introductory tutorial only requires
pip install mesa
whereas the visualization tutorial requirespip install "mesa[viz]"
. This may seem obvious but not if someone jumpstarts to the tutorials without going to "Installation Options" section on the documentation index.Comments on the visualization tutorial (@tpike3)
In the visualization tutorial, I would put the whole code of "MoneyModel.py" within the jupyter notebook, removing the need to search through the first tutorial for a file that is not really described.
Maybe mention that the visualization needs the "rec" dependencies in the installation?
I find the "Choose version" button at the top confusing, as it does not display the currently chosen version and there is already an indication on the bottom right.
[from Marti] Similar to the previous point, it's difficult to know which MoneyModel to use in the visualization tutorial. Many MoneyModel classes are defined in the introductory tutorial, so it would be better to include the class definition in the visualization tutorial, even if repetitive.
Comments on the code
The quality of the code and tests seem very good!
Typing
(split off in Missing Agent type hints #2716)
[from Marti] Question about packaging: The source distribution is quite big (2.4 MB), is that intended? In most Python libraries the docs are excluded from the source distribution (the docs account for 2.5 out of the total 3.4 MB). Also, are the basic examples packaged as a core mesa module for a particular reason?
Resolved in Clean-up old images, compress wolf-sheep image, remove version switch artifact #2717
Comments on the paper
l16 The Statement of Need could be more clearly structured. Currently, it reads more like a historical recap. I recommend organizing it along these lines: (1) Why such a package is needed, (2) The current state of the field, and (3) What MESA contributes — focusing on its present state rather than its 2014 origins. Additionally, please refer to the Statement of Need and State of the Field requirements outlined in the JOSS checklist above to ensure alignment with the journal's expectations. (@jackiekazil)
l41 For consistency with the other sections, please add a code example of instantiating a model and passing a seed paper: Add code examples and clarify terminology #2719
l46 Similarly, please add a code example of instantiating and subclassing an agent paper: Add code examples and clarify terminology #2719
l80 & l86 Maybe call these two 'modes' "discrete time" and "continuous time", if I understood it correctly? I think that would make it easier to understand. paper: Add code examples and clarify terminology #2719
l109 I would suggest to move the "Applications" section to the start of the paper (@jackiekazil)
l134 Acknowledgements are usually placed after the conclusions (@jackiekazil)
[from Marti] Review of related tools is missing. Besides agentpy, potential candidates include Melodie (which also has a JOSS paper), helipad, pythonabm, and bptk_py. These merit consideration and should potentially be referenced in the paper.
[from Marti] Typos and minor syntactic comments: (@jackiekazil)
Smaller comments on the paper
l15 Change to 2025 (@wang-boyu)
l17 Not all ABMs represent artificial societies, here it is an implied synonym (@wang-boyu)
l20 The two references need to be placed into the same bracket (@wang-boyu)
l28 I think it should say here: "an" improved visualization framework (@wang-boyu)
l34 What does pure python mean? Does it not also use C, JS/React, etc.? (@jackiekazil)
l35 Markdown mistake in list formatting (@wang-boyu)
l43 Word repetition: "instantiates" (@wang-boyu)
l46 Please add a sentence describing agent types, to understand what the types mean later at "Data collection" paper: Add code examples and clarify terminology #2719
l62-64 Please shortly explain these terms or add a reference - (Requested more info on 02/13 here: [REVIEW]: Mesa 3: Agent-based modelling with Python in 2024 openjournals/joss-reviews#7668 (comment)) paper: Add code examples and clarify terminology #2719
l94 Markdown mistake, close bracket for link (@wang-boyu)
l96 Please add a link or reference regarding the Solara package (@jackiekazil)
l97 Please use a code example and screenshot that correspond to each other, demonstrating what kind of code creates what kind of visualization. paper: Add code examples and clarify terminology #2719
l98 Maybe add a sentence how this visualization module aligns or differs with the established standard of NetLogo.
l126 Markdown mistake in list formatting (@wang-boyu)
l133 Maybe add a sentence on which range of topics are covered by these community extensions and add a link or reference to a list or overview of these extensions and tutorials same as with the example models paper: Add code examples and clarify terminology #2719
l143 Maybe say "want" instead of "need"? (@jackiekazil)
The text was updated successfully, but these errors were encountered: