[JOSS Review] Installation, documentation, issues/contributing, manuscript content

[avapolzin]

Hi, everyone,

Here are some initial comments on the installation instructions, documentation, issues/contributing, and manuscript content as part of the JOSS review.

Installation:

• Is there a reason that the pip installation from PyPI is not listed in the README? As it exists and may be preferred by some users, it should be included in the installation instructions. If the concern is with environment creation or direct dependencies, this can be stated in the documentation, but right now there’s an orphaned PyPI entry.
• Within the pyproject.toml file, it looks like an old email is used for the first author. Assuming that email is still accessible/checked, this is not a problem, but it should be updated if it won’t be monitored.
• Users should not have to go into environment.yml to understand the environment name, as they will need to know it to install and then use DeepSZSim. This should be spelled out as e.g.,

conda env create -f environment.yml
conda activate szsims
pip install .

Documentation:

• The README is very sparse, and it will need to be fleshed out, as the Read the Docs page points to it. At the same time, the Read the Docs should be standalone and need to have sufficiently detailed information to serve as documentation (otherwise users could just look at your docstrings). Generating a README-like .rst file for sphinx is relatively trivial, and it would also be good to include usage examples or similar in the official documentation.
• At no point is the Read the Docs page listed in the available README — much like the absent PyPI reference despite an existing record, it will be a confusing omission for users. Please link directly to the Read the Docs page from your README (the Read the Docs badges are very nice for this and offer a check that the documentation compiled properly).
• Within deepszsim.readthedocs.io, some of the docstrings were not read in properly by sphinx. Please check the formatting to ensure that it is uniform and readable. Similarly, some of the functions are missing descriptions. Please add those to each function regardless of whether they are user-facing or not per PEP 8.

Issues/Contributions:

• As there are quite a few authors, it would be appropriate to be clear about who exactly should be contacted about contributions. If it is, in fact, all of the authors, it should be made explicit. If there are authors who are specifically charged with maintaining the software, please list explicitly who users should contact as this will facilitate engagement. Currently, only some authors even have websites linked, so please also include emails (it can be in name [at] institution [dot] edu format) for the relevant contacts.
• There should be a formalized procedure for bug reports, too. Many people will default to opening an issue, but please also be clear about whether you want direct contact (i.e., emails — and where they should go) or if issues are sufficient.
• There are many open issues in the repository currently — a quick glance through suggests that many of these should be closed as completed. Maintainers should go through and close completed issues and address any open ones that are pressing and critical to the project. This can exclude wishlist items, but with a substantial backlog of open issues, it will appear like the project is not being actively maintained. (If you would like a forum to publicly engage with authors about improvements to the code, consider using the discussions feature on GitHub to avoid muddying the purpose of issues by leaving so many open.)

Paper/Paper Draft:

• I’m sure this will be addressed by the editors, but some of the references in the paper are not processed properly in the resultant PDF (from “DeepCMBSim Module” on) because you have enclosed the references in backticks. This will be trivial to address.
• On occasion, in-text references are used when the citations should be parenthetical and vice versa. The paper should be proofread to fix these instances.
• Line 82, “Figure Figure 2”.
• It does not appear that DeepCMBSim has been referenced (whether formally or via footnote linking to the parent repo). As this is the basis of your code, it is critical that this properly credited.
• CAMB should be cited on first reference.
• Line 89, M_{200}.
• Line 101, "Figure Figure 3".

I will follow up soon with comments about the code and functionality.

[samueldmcdermott]

we are working on this now @avapolzin! One quick question -- you say

Within deepszsim.readthedocs.io, some of the docstrings were not read in properly by sphinx. Please check the formatting to ensure that it is uniform and readable. Similarly, some of the functions are missing descriptions. Please add those to each function regardless of whether they are user-facing or not per PEP 8.

but things look ok to me. Can you provide a link to a particular example and I can go from there? thank you!

[avapolzin]

Hi, @samueldmcdermott -- just a couple of functions seem to have the issue, but the arguments are not all properly separated in e.g., deepszsim.filters.get_tSZ_signal_aperture_photometry() and deepszsim.visualization.plot_img(). Similarly, the rest of deepszsim.visualization is missing details of the arguments in the docstrings.

[bnord]

Hi @avapolzin Thanks for your review comments. I have a question about the open issues. I think it will be a while 'til we get to the wish list items. Do you recommend that move those into one list in a discussion?

[avapolzin]

Hi, @bnord -- I think it will be a "cleaner" look to move wish list items to Discussions. Whether you want to consolidate them or not may rest on the timeline you foresee/whether some of them are related to each other; my feel is that should be determined by whatever works best for the collaboration/organization.

[evavagiakis]

Hi @avapolzin thank you very much for these valuable comments and for your patience, we had some personnel changes over the past few months. We have addressed these comments as follows:

Installation:

• The pip installation from PyPI is now listed in the README.
• My Cornell email address is still valid, I'm a Visiting Scientist at Cornell and use the address for GitHub, so I have elected to keep this address for this project.
• We have followed your suggestion and updated the scripts so that the user does not need to go into environment.yml to understand the environment name.

Documentation:

• We have made improvements to flesh out the README.
• We have added a direct link to the Read the Docs page in the README.
• We have checked the formatting to ensure that it is uniform and every function now has a docstring.

Issues/Contributions:

• We have clarified this in the paper by adding "For contributions, questions, or maintainance, contact: E. M. Vavagiakis (ev66 [at] cornell [dot] edu), S. D. McDermott (sammcd [at] uchicago [dot] edu), B. Nord (nord [at] fnal [dot] gov) or C. Avestruz (cavestru [at] umich [dot] edu)." to the Acknowledgements section.
• The procedure for bug reports has been clarified to be opening an issue.
• We have gone through and cleaned up open issues and moved relevant threads to Discussions.

Paper/Paper Draft:

We have made all the requested changes in the paper draft.

[avapolzin]

Thank you, @evavagiakis.

This largely addresses my comments, but there are a handful of small additional changes that I would recommend making to improve the accessibility of the code:

• add the example notebooks to the RTD site via nbsphinx and nbsphinx_link -- this is a good alternative to more extensive documentation (though I would still suggest having your preferred installation instructions listed there, too)
• add a "long description" to PyPI, which at minimum should include a short note about what the code does with link to the docs and repository or can be a full copy of your README
• Line 76, [@\Hu:2001kj] -> @\Hu:2001kj
• Line 85-86, Battaglia et al. 2012 [@\Battaglia:2012] -> @\Battaglia:2012
• Line 94, Plank:2019 reference is missing; I imagine this is either Planck:2018 or Planck:2018vyg

Some of the journal names in the references include HTML codes and other artifacts from the BibTeX, and I would suggest a a glance over the references, but as many of these will be caught primarily at the editorial stage, it's more of a long term note and not something that I consider critical to address for the review specifically.