[REVIEW]: Pynamical: Model and Visualize Discrete Nonlinear Dynamical Systems, Chaos, and Fractals

whedon commented 6 years ago

Submitting author: @gboeing (Geoff Boeing) Repository: https://github.com/gboeing/pynamical Version: 0.1.2 Editor: @labarba Reviewer: @drvinceknight, @sdross0 Archive: 10.5281/zenodo.1294299

Status

Status badge code:

HTML: <a href="http://jose.theoj.org/papers/8e0d222df1d4e63dcf61bebcb643714a"><img src="http://jose.theoj.org/papers/8e0d222df1d4e63dcf61bebcb643714a/status.svg"></a>
Markdown: [![status](http://jose.theoj.org/papers/8e0d222df1d4e63dcf61bebcb643714a/status.svg)](http://jose.theoj.org/papers/8e0d222df1d4e63dcf61bebcb643714a)

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) 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

@drvinceknight, @sdross0, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

Make sure you're logged in to your GitHub account
Be sure to accept the invite at this URL: https://github.com/openjournals/jose-reviews/invitations

The reviewer guidelines are available here: https://jose.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @labarba know.

Review checklist for @drvinceknight

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSE conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSE code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Version: Does the release version given match the GitHub release (0.1.2)?
[x] Authorship: Has the submitting author (@gboeing) made substantial contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation? (and documentation is sufficient?)
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state the need for this software and who the target audience is?
[x] Installation instructions: Is there a clearly stated list of dependencies? (Ideally these should be handled with an automated package management solution.)
[x] Example usage: Do the authors include examples of how to use the software?
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Tests: Are there automated tests or manual steps described so that the function of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Authors: Does the paper.md file include a list of authors with their affiliations?
[x] A statement of need: Do the authors clearly state the need for this software and who the target audience is?
[x] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @sdross0

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSE conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSE code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Version: Does the release version given match the GitHub release (0.1.2)?
[x] Authorship: Has the submitting author (@gboeing) made substantial contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation? (and documentation is sufficient?)
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state the need for this software and who the target audience is?
[x] Installation instructions: Is there a clearly stated list of dependencies? (Ideally these should be handled with an automated package management solution.)
[x] Example usage: Do the authors include examples of how to use the software?
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Tests: Are there automated tests or manual steps described so that the function of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Authors: Does the paper.md file include a list of authors with their affiliations?
[x] A statement of need: Do the authors clearly state the need for this software and who the target audience is?
[x] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

whedon commented 6 years ago

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @drvinceknight it looks like you're currently assigned as the reviewer for this paper :tada:.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/jose-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

Set yourself as 'Not watching' https://github.com/openjournals/jose-reviews:

watching

You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

whedon commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

drvinceknight commented 6 years ago

Proof looks good, I will complete the review before the weekend.

drvinceknight commented 6 years ago

I'm (really) impressed with the software and the paper is well written.

A couple of minor things that I have not checked off on my list and would like to run past @gboeing:

Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?

The documentation on github is great: I ran everything and it worked exactly as expected.

I noted that the documentation rendered by sphinx isn't complete. The autodoc of the api doesn't seem to have picked up the various methods/modules: https://pynamical.readthedocs.io/en/latest/pynamical.html#submodules

Tests: Are there automated tests or manual steps described so that the function of the software can be verified?

The test suite is currently just a test of run to completion of the various functions and not of expected output. I don't think this is something that should hinder publication at all but perhaps @gboeing you would want to think about adding some tests of expected output:

For example:

def test_phase_diagram_3d():    
    pops = simulate(model=cubic_map, num_gens=200, rate_min=3.5, num_rates=30, num_discard=100)
    phase_diagram_3d(pops, xmin=-1, xmax=1, ymin=-1, ymax=1, zmin=-1, zmax=1, alpha=0.2, color='viridis', azim=330, 
                     legend=True, save=False, folder=_img_folder, filename='')
    phase_diagram_3d(pops, xmin=-1, xmax=1, ymin=-1, ymax=1, zmin=-1, zmax=1, alpha=0.2, color='inferno', azim=330, 
                     legend=True, remove_ticks=False, save=False, folder=_img_folder, filename='')

we could at least assert the type of pops (that it returns a dataframe) and possibly also the shape of the dataframe? Similarly for phase_diagram_3d (we can assert that we get a tuple, and the type of each element).

Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

I didn't see these in the repo but perhaps I missed them?

gboeing commented 6 years ago

@drvinceknight thank you for these thoughtful recommendations. I have made each of these changes you suggested:

sphinx autodoc now renders all the docstrings properly: https://pynamical.readthedocs.io/en/latest/pynamical.html#submodules
new tests accordingly added in https://github.com/gboeing/pynamical/commit/861f9cec2608c0ffe7efdd588ae2f55878c3ca18
community contribution, issues, and support guidelines added in https://github.com/gboeing/pynamical/commit/689cea0ef4b30364091ffc8b2399e7e521fec45c

drvinceknight commented 6 years ago

Thanks for those @gboeing: it all looks great to me know and I gladly recommend this for publication. A very nice library, I might make use of it myself at some point (will let you know if/when I do!).

gboeing commented 6 years ago

@drvinceknight thanks!

labarba commented 6 years ago

Thanks for your review, @drvinceknight 👏

@sdross0 — Since we have a thorough review of the software side by @drvinceknight, I encourage you to focus your review on the educational value of the package, and especially go over the demos/tutorial and see if you have any recommendations there. As for installation, etc., you could have a student in your group install it, to get a double check and tick off those items on the review checklist. Thank you!

sdross0 commented 6 years ago

Pros: The software installs as advertised through both methods, and is able to successfully run all examples presented. Quick set-up and first examples worked out of the box. It runs very fast, which impresses me a lot. Very nice.

Cons: From the docs, it's not clearly explained what class of problems this is designed to work for. Nonlinear dynamical systems could be any number of things (n-dimensional mappings, systems of ordinary differential equations, time-delay equations, etc.)

It seems to be designed primarily for 1D maps, that is, 1-dimensional discrete dynamical systems of the form

x_k+1 = f(x_k)

which isn't explicitly described in the documentation. I think a description of the kinds of models that the software has in mind will make it more successful. It would also help to know how to input your own models into the system.

Related to that, I worry the software might not generalize well to the other types of dynamical systems that would appear in an introductory course on dynamical systems (say, following Strogatz book), mostly because of the restriction to 1D maps. Most such courses do include 1D maps at the beginning of the course, and perhaps that is where Pynamical could be most useful.

labarba commented 6 years ago

@gboeing See how you can address the comments above in the documentation, examples and paper, and let us know. You may want to add a note about the current limitations of Pynamical, whether and how it could be expanded beyond those limitations, and how it might be used in a standard course in dynamical systems.

gboeing commented 6 years ago

@sdross0 thank you for these thoughtful recommendations. I have made each of these changes you suggested:

clarified the software's focus on 1-D maps in the documentation in https://github.com/gboeing/pynamical/commit/22a3e7fb70456891865837f3b86db8de538bbb09 and readme in https://github.com/gboeing/pynamical/commit/2a1cad13d71350401f6c91c20580e019c54eb9cf
identified where this software could be most useful, accordingly, in the beginning of an intro course or demonstrating patterns to students in less technical fields, in https://github.com/gboeing/pynamical/commit/471f11ef2ffa0914901d0ffb21eb4e05e301cddb
highlighted example of how to define/input your own model in https://github.com/gboeing/pynamical/commit/2a1cad13d71350401f6c91c20580e019c54eb9cf (example demonstrates defining the mandelbrot map then inputting it for simulation/visualization)

labarba commented 6 years ago

@drvinceknight, @sdross0 — Thank you both for reviewing this submission to JOSE. Since you both have ticked all items in your checklist, and made constructive comments that were addressed, all I need now is a confirmation on a comment below that you are ready to recommend acceptance.

This will be the first paper in JOSE and we're excited and grateful to you for being a part of this plunge into a new model of scholarly publication!

drvinceknight commented 6 years ago

now is a confirmation on a comment below that you are ready to recommend acceptance.

I confirm :+1:

This will be the first paper in JOSE and we're excited and grateful to you for being a part of this plunge into a new model of scholarly publication!

Awesome: :+1:

sdross0 commented 6 years ago

I recommend acceptance.

labarba commented 6 years ago

@gboeing The next step is for you to make an archive on a service like Zenodo, get a DOI, and post the DOI here. (You may want to tag a release on your GitHub repo right before archiving.)

gboeing commented 6 years ago

@labarba 10.5281/zenodo.1294299

labarba commented 6 years ago

@whedon set 10.5281/zenodo.1294299 as archive

whedon commented 6 years ago

OK. 10.5281/zenodo.1294299 is the archive.

whedon commented 6 years ago

:tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:

If you would like to include a link to your paper from your README use the following code snippet:

[![DOI](https://jose.theoj.org/papers/10.21105/jose.00015/status.svg)](https://doi.org/10.21105/jose.00015)

This is how it will look in your documentation:

We need your help!

Journal of Open Source Education is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

Volunteering to review for us sometime in the future. You can add your name to the reviewer list here: http://jose.theoj.org/reviewer-signup.html
Making a small donation to support our running costs here: https://www.flipcause.com/secure/cause_pdetails/Mjk3ODA=

labarba commented 6 years ago

@gboeing -- your paper is now published in JOSE ‼️ Congratulations 🎉 You can now add the green happy JOSE button on your repository.

@drvinceknight, @sdross0 — Thank you again for your review 🙏

openjournals / jose-reviews