JOSS review (adrn)

[adrn]

Ref: openjournals/joss-reviews#3675

Hi @mikegrudic 👋 ! Overall things look good here -- I'm looking forward to using this package myself! -- but I do have some suggestions and comments below.

Installation

I did not find any instructions for installation. I recommend that the authors add an "Installation" section to the README or documentation (see below), which could either contain the necessary information or link to an INSTALL file that describes how to install the package. For an example: https://github.com/adrn/gala#installation-and-dependencies

(I was able to pip install pytreegrav, which successfully installed a wheel)

Functionality

I successfully ran the walkthrough code on my machine after installing the package.

Performance

I have verified the scaling claims (Figure 1 in JOSS article) on my machine (MacBook Pro laptop).

Documentation

The documentation is in the form of a section of the repository README. My main comment would be to separate the documentation from the README. At maximum, switch to using a documentation engine like Sphinx or MkDocs built and served on a service like Readthedocs, and link to this documentation from your README. At minimum, I would recommend making a docs/ directory, moving your current README.ipynb to docs/walkthrough.ipynb, and link to this from the README.txt file. I would also recommend removing the code/walkthrough from the README.txt itself and instead link to the walkthrough IPython notebook (so you avoid duplicating that text/code).

Other comments:

• I don't see any mention of what profile is used to soften the point mass potentials when h is provided. Is there a standard profile to use (i.e. Plummer?), or is this configurable? It would be good to explicitly state this in the documentation.
• As mentioned above, at minimum, please add an "Installation" section to the README that links to a file that contains installation instructions (even if it just states explicitly that the way to install is with pip)
• Some users might find it useful to have API documentation, so it's worth considering whether you want to write this yourself or build it automatically with an automatic documentation build system.
• Please add some documentation of how a user can run tests of the package. It looks like there is a minimal test in tests/test.py, but I would recommend switching to a pytest or nosetests-compatible test layout.
• It looks like there is an associated github pages build of the README: Please link to this page (maybe with a badge link?) at the top of your README, as this serves as your main documentation.
• Please add some documentation of how other users can contribute or engage with the developers. In particular, how do users 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support. An example statement: http://gala.adrian.pw/en/latest/contributing.html

General comments

• Many repositories on GitHub (including GitHub itself) are switching from a default branch named "master" to "main" (see, e.g., https://github.com/github/renaming). You might want to consider renaming the default branch (but it's up to you!).
• My understanding from reading the README and walkthrough is that the primary use case for this code is for re-analyzing simulation output (i.e. not for users who actually want to run simulations, as the code operates at the Python level, using numba to accelerate things). It might be worth having a few scientific case studies in the walkthrough that put the utilities in this package in context of some "real world" examples.
• Though the details of code style can be subjective, Python packages generally at least follow the convention (laid out in PEP 8) that class names follow the CapWords convention and function names are lower case. Please consider reformatting the functions in this package to follow PEP8 guidelines.

[mikegrudic]

Hi @adrn, see response here: https://github.com/openjournals/joss-reviews/issues/3675#issuecomment-977252603