search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
721 stars 38 forks source link

[REVIEW]: MacroModelling.jl: A Julia package for developing and solving dynamic stochastic general equilibrium models #5598

Closed editorialbot closed 1 year ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@thorek1<!--end-author-handle-- (Thore Kockerols) Repository: https://github.com/thorek1/MacroModelling.jl Branch with paper.md (empty if default branch): joss Version: v0.1.29 Editor: !--editor-->@jbytecode<!--end-editor-- Reviewers: @gdalle, @jmejia8 Archive: 10.5281/zenodo.8374466

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/1a469cf03f3c1fd96db74f97a02c493c"><img src="https://joss.theoj.org/papers/1a469cf03f3c1fd96db74f97a02c493c/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/1a469cf03f3c1fd96db74f97a02c493c/status.svg)](https://joss.theoj.org/papers/1a469cf03f3c1fd96db74f97a02c493c)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@Athene-ai & @daviddewhurst & @gdalle, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @jbytecode know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Checklists

📝 Checklist for @gdalle

📝 Checklist for @jmejia8

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

thorek1 commented 1 year ago

Opened a PR for some minor formatting / syntax details, and then as far as I'm concerned this is good to go

thanks for the suggestions. I merged the commit and fine-tuned them a bit more in the latest commits

jmejia8 commented 1 year ago

@thorek1 - Firstly, thank you and congrats about the package. The software seems useful to model and analyze certain economic problems.

Below are my initial comments regarding my review of the paper and codebase:

  1. The installation proceeded without problems. I want to mention that pre-compilation stage took several minutes on my computer (which can be expected); however, importing the package also require some time, in respect to other packages. Could you explain why is this happening?
  2. The automated tests seem covering most of the functionalities, but take a long time to complete.
  3. I did not find information on the community guidelines for those willing to contribute.
  4. The paper does not have the section 'Statement of need'.
  5. The package is compared to related software in an informative manner. I suggest citing the papers related to the software (if possible), besides including the link to the software.

Due to I'm not an expert on the field, the following comments come from a colleague of mine who is an economist:

  • Most business cycle research focuses on the behavior of the representative agent model and neglects the effects that are caused by the heterogeneity of agents. So how does the software package proposal address this issue?
  • Moreover, the software package proposal seems to work only for time constraints on a single/representative agent. Why not to consider the usual case of a macroeconomic model based on the stochastic Overlapping Generations (OLG) theory where households maximize discounted life-time utility with regard to their intertemporal consumption, capital and money demand, and labor supply !? Would it work?
thorek1 commented 1 year ago

@jmejia8 thank you for your comments

  1. the codebase is fairly large with 3800 lines (vs Plots.jl with 8000). furthermore for each model the equations are parsed (including for precompilation). all taken together it takes time and I tested different ways of trying to speed it up and the current amount of precompilation is the best answer i have so far.
  2. they take a long time because all 16 currently implemented models are tested on most functionalities. a model is estimated (takes at least 20 minutes). this extensive testing actually helps a lot in debugging.
  3. i will add those
  4. the paper used to have it (see previous paper compilations further up in the thread) but @gdalle and I agreed to drop it and have the content spread out in other sections of the paper. let me know if you prefer otherwise
  5. I will add them where applicable

Regarding the comments from your colleague:

jbytecode commented 1 year ago

@jmejia8 - Thank you for your suggestions. It seems our author has changed and corrected something and these changes are ready for your further review. Thank you in advance and sorry for bothering and being to much verbose.

jmejia8 commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

thorek1 commented 1 year ago

I added community guidelines at the end of the readme and cited papers relating to mentioned packages in the paper if applicable.

jmejia8 commented 1 year ago

@editorialbot generate pdf

jmejia8 commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jmejia8 commented 1 year ago

@thorek1 - Thank you for the answers, all clear now. Below are my last comments on the paper:

  1. Regarding "Statement of need", it is not clear for me who the target audience is (economists, students, scientist, practitioners?) after reading the paper. It would be better to include this information explicitly, maybe in the first section.
  2. It could be better to cite the peer-reviewed paper on Julia (https://doi.org/10.1137/141000671) instead of the technical report.

Although the package has performance issues, it seems useful and works for what it was designed to do, which is the most important IMO. Documentation has many examples, the API is detailed, and installation proceeds as it should. Therefore, I do not have further comments on the software.

jbytecode commented 1 year ago

@jmejia8 - I appreciate your help since I was having difficulties finding a second suitable reviewer for this submission.

@thorek1 - Please consider applying/adding/correcting the issues stated by our reviewer and ping me when you've done with them.

Thank you in advance.

jmejia8 commented 1 year ago

@jbytecode - It is my pleasure to review for JOSS.

thorek1 commented 1 year ago

I added the target audience to the paper and changed the reference to the peer-reviewed Julia paper.

Thank you @jmejia8 for your review.

jbytecode commented 1 year ago

@editorialbot generate pdf

@jmejia8 - could you please check off the changes and finalize your review?

Thank you in advance.

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jmejia8 commented 1 year ago

@jbytecode - I just completed my review. Congrats to @thorek1 for the nice package.

jbytecode commented 1 year ago

@gdalle - Now it's your turn. Could you please check off the latest changes and finalize your review?

Thank you in advance

gdalle commented 1 year ago

all good on my end

jbytecode commented 1 year ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

jbytecode commented 1 year ago

@editorialbot check references

editorialbot commented 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1137/141000671 is OK
- 10.1111/iere.12306 is OK

MISSING DOIs

- 10.2139/ssrn.2364989 may be a valid DOI for title: Fifth-Order Perturbation Solution to DSGE models
- 10.2139/ssrn.2602453 may be a valid DOI for title: Efficient perturbation methods for solving regime-switching DSGE models
- 10.3386/w30573 may be a valid DOI for title: Differentiable State-Space Models and Hamiltonian Monte Carlo Estimation

INVALID DOIs

- https://doi.org/10.1016/j.red.2023.01.001 is INVALID because of 'https://doi.org/' prefix
jbytecode commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jbytecode commented 1 year ago

@thorek1 - I've just sent a pull request (https://github.com/thorek1/MacroModelling.jl/pull/51) of some minor changes in the bibliography file. Please review the changes and apply. Thank you in advance.

jbytecode commented 1 year ago

@thorek1 - The JOSS documentation located in URL https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain states that your paper should include a Statement of need section that clearly illustrates the research purpose of the software and places it in the context of related work. However, the manuscript in its current form doesn't have a section labeled with that. Could you please correct this issue? (possibly a refactoring would solve this).

gdalle commented 1 year ago

It's my fault, during review I suggested to merge it with the summary to improve exposition. I didn't realize the words "statement of need" had to be present

jbytecode commented 1 year ago

@gdalle - No worries, that's why we have an editorial control system.

thorek1 commented 1 year ago

@thorek1 - The JOSS documentation located in URL https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain states that your paper should include a Statement of need section that clearly illustrates the research purpose of the software and places it in the context of related work. However, the manuscript in its current form doesn't have a section labeled with that. Could you please correct this issue? (possibly a refactoring would solve this).

I resurrected the Statement of need section and merged your PR (many thanks).

thorek1 commented 1 year ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

  • [ ] Double check authors and affiliations (including ORCIDs)
  • [ ] Make a release of the software with the latest changes from the review and post the version number here. This is the version that will be used in the JOSS paper.
  • [ ] Archive the release on Zenodo/figshare/etc and post the DOI here.
  • [ ] Make sure that the title and author list (including ORCIDs) in the archive match those in the JOSS paper.
  • [ ] Make sure that the license listed for the archive is the same as the software license.

Editor Tasks Prior to Acceptance

  • [ ] Read the text of the paper and offer comments/corrections (as either a list or a PR)
  • [ ] Check the references in the paper for corrections (e.g. capitalization)
  • [ ] Check that the archive title, author list, version tag, and the license are correct
  • [ ] Set archive DOI with @editorialbot set <DOI here> as archive
  • [ ] Set version with @editorialbot set <version here> as version
  • [ ] Double check rendering of paper with @editorialbot generate pdf
  • [ ] Specifically check the references with @editorialbot check references and ask author(s) to update as needed
  • [ ] Recommend acceptance with @editorialbot recommend-accept

Here is my progress:

Additional Author Tasks After Review is Complete

jbytecode commented 1 year ago

@editorialbot check references

editorialbot commented 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1093/acprof:oso/9780199641178.001.0001 is OK
- 10.1093/restud/rdx037 is OK
- 10.1137/141000671 is OK
- 10.1016/j.jedc.2017.04.007 is OK
- 10.2307/1913386 is OK
- 10.2139/ssrn.2602453 is OK
- 10.1111/iere.12306 is OK
- 10.1016/j.red.2023.01.001 is OK
- 10.3386/w30573 is OK

MISSING DOIs

- None

INVALID DOIs

- None
jbytecode commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jbytecode commented 1 year ago

@thorek1 - Please fulfill the requirements given below:

thank you in advance.

jbytecode commented 1 year ago

@editorialbot check references

editorialbot commented 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1093/acprof:oso/9780199641178.001.0001 is OK
- 10.1093/restud/rdx037 is OK
- 10.1137/141000671 is OK
- 10.1016/j.jedc.2017.04.007 is OK
- 10.2307/1913386 is OK
- 10.2139/ssrn.2602453 is OK
- 10.1111/iere.12306 is OK
- 10.1016/j.red.2023.01.001 is OK
- 10.3386/w30573 is OK

MISSING DOIs

- None

INVALID DOIs

- None
jbytecode commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jbytecode commented 1 year ago

@editorialbot set v0.1.29 as version

editorialbot commented 1 year ago

Done! version is now v0.1.29

jbytecode commented 1 year ago

@editorialbot set 10.5281/zenodo.8374466 as archive

editorialbot commented 1 year ago

Done! archive is now 10.5281/zenodo.8374466

thorek1 commented 1 year ago
jbytecode commented 1 year ago

@editorialbot check references

editorialbot commented 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1093/acprof:oso/9780199641178.001.0001 is OK
- 10.1093/restud/rdx037 is OK
- 10.1137/141000671 is OK
- 10.1016/j.jedc.2017.04.007 is OK
- 10.2307/1913386 is OK
- 10.2139/ssrn.2602453 is OK
- 10.1111/iere.12306 is OK
- 10.1016/j.red.2023.01.001 is OK
- 10.3386/w30573 is OK

MISSING DOIs

- None

INVALID DOIs

- None
jbytecode commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jbytecode commented 1 year ago

@thorek1 - In line 50 of the manuscript, the row is rendered as

steady states symbolically, a feature shared only with [gEcon](http://gecon.r-forge.r-project.org

Could you please correct and ping me again?