search

openjournals / joss-reviews

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

[REVIEW]: The Python Sky Model 3 software #3783

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @zonca (Andrea Zonca) Repository: https://github.com/galsci/pysm Version: v3.3.2 Editor: @christinahedges Reviewer: @patricialarsen, @smsharma Archive: 10.5281/zenodo.5620868

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

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

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

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

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

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @christinahedges 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 ✨

Review checklist for @patricialarsen

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @smsharma

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @patricialarsen, @smsharma it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-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:

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

watching

  1. 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

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 3 years ago

Wordcount for paper.md is 1150

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1086/427976 is OK
- 10.21105/joss.01298 is OK
- 10.1093/mnras/stx949 is OK
- 10.1088/0067-0049/208/2/20 is OK
- 10.1145/2833157.2833162 is OK
- 10.1051/0004-6361/201321494 is OK
- 10.1051/0004-6361/201322068 is OK
- 10.3847/1538-3881/aabc4f is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.11 s (648.0 files/s, 107392.1 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
Python                           42            820           1400           2965
Jupyter Notebook                  8              0           4416            527
reStructuredText                  7            148             28            285
TeX                               1              8              0            186
DOS Batch                         1             21              1            150
make                              1             22              5            106
INI                               1             14              0             90
YAML                              3             12             18             87
Markdown                          1             22              0             74
Bourne Shell                      2              0              0             12
Bourne Again Shell                1              1              5              5
TOML                              1              2              0              5
--------------------------------------------------------------------------------
SUM:                             69           1070           5873           4492
--------------------------------------------------------------------------------

Statistical information for the repository '7fb1e252e6e09ab7c8ccda6e' was
gathered on 2021/09/29.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Andrea Zonca                   218          6873           3271           79.11
Ben Thorne                      21           706            173            6.86
NicolettaK                       1            21             15            0.28
Xavier Garrido                   3            51             27            0.61
ben thorne                      11          1345            340           13.14

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Andrea Zonca               4696           68.3         23.0               12.18
Ben Thorne                  431           61.0         26.5               15.08
NicolettaK                   17           81.0         11.5                0.00
Xavier Garrido               41           80.4         16.1                4.88
whedon commented 3 years ago

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

christinahedges commented 3 years ago

@zonca, @patricialarsen, @smsharma – This is the review thread for the paper. Please don't hesitate to message me here if you have questions.

Please read the "Reviewer instructions & questions" in the first comment above to get started. If you get lost, you can also see the reviewer guidelines.

Both reviewers have checklists at the top of this thread (in that first comment) with the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. If you are concerned about a requirement, please discuss it here on this thread 🧵 . Feel free to post about questions/concerns as they come up as you go through your review.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#3783 so that a link is created to this thread (and I can keep an eye on what is happening).

We aim for the review process to be completed within about 4-6 weeks but please make a start well ahead of this as JOSS reviews are by their nature iterative and any early feedback you may be able to provide to the author will be very helpful in meeting this schedule. When you're finished with your checklist, leave a comment and @ me to let everyone know you're review is complete.

patricialarsen commented 3 years ago

Hi!

Installation is going fairly smoothly so far. The conda, pip and development installation work from the instructions on my home linux computer, with a fresh python3 installation.

A few minor issues though once I hit running the unit tests on these - firstly there's additional notes on this in the setup.py file. Those are useful but a little well-hidden and could be placed somewhere more visible. Secondly a slight issue is coming up in the config file for an install of the development version into a fresh conda environment:

I'm getting pytest: error: unrecognized arguments: --doctest-rst

It works okay if I change the flag in line 45 of setup.cfg from addopts = --doctest-rst to addopts = --doctest-glob="*.rst"

I'm having a little more trouble getting the tests running through tox, which seem to be fixed by adding dependencies to the config file as well as updating my c compilers through conda install -c conda-forge c-compiler compilers cxx-compiler

The dependencies I had to add to the deps section of tox.ini are: astropy pytest numba healpy

I then manage to pass tests using tox with =================================== 62 passed, 3 skipped, 585 warnings in 55.26s =================================== And through pytest with =================================== 60 passed, 2 skipped, 340 warnings in 52.24s ===================================

A large number of warnings, which mostly look like UserWarning: Retrieve data for pysm_2_test_data/check_d5_100_uK_RJ_64.fits (if not cached already)

These warnings could be looked into, and suppressed if they're not important to avoid flagging as an issue.

@smsharma when you look at the installation/automated tests could you please keep an eye out for if these tests run straightforwardly on your system and if you need any adjustments to get them to run?

zonca commented 3 years ago

thanks @patricialarsen I started to work on your recommendations.

About tox, I cannot reproduce the problem.

I tried with a plain conda environment with only python 3.8. I ran:

tox -e test

and it worked fine. How are you testing it?

patricialarsen commented 3 years ago

Hi @zonca,

I can confirm that the latest github version fixes the pytest issues on my end :)

Re the tox issues, it was failing to read the test dependencies correctly, I can't reproduce it right now oddly after re-cloning the github and restarting the computer, which means it was likely either a temporary issue in my .tox files or environment, or how I was running it at the time and probably nothing that needs changing. Assuming the other reviewer manages to run them okay it should be fine I think.

whedon commented 3 years ago

:wave: @smsharma, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @patricialarsen, please update us on how your review is going (this is an automated reminder).

christinahedges commented 3 years ago

Hi @patricialarsen and @smsharma, I think the last comments were two weeks ago, just wanted to check in how your reviews are coming? It's ok if you need some more time, but maybe drop a comment if you feel you will need a lot more time. @smsharma we haven't heard from you on the thread yet so just wanted to check in.

smsharma commented 3 years ago

Hi @christinahedges, sincere apologies for the delay, I will need a bit more time. I will aim to complete the review by the start of next week.

patricialarsen commented 3 years ago

@christinahedges sorry for the delay, I'll get my next round of comments in before the weekend

smsharma commented 3 years ago

Hi! Congrats to the authors and contributors first of all for a very nice package that's extremely useful to the CMB community.

Installation

I tested the package on an HPC cluster (running CentOS Linux 7) starting from a clean conda environment. I was able to install it no problem through conda and pip, as well as the development version using instructions provided in the documentation.

Tests

I had to install tox and pytest separately. While this is straightforward to do, a minor suggestions would be to either specify that these need to be installed separately, or including them as requirements.

Building the docs locally required installing pandoc; this could be briefly mentioned in the repo README.rst where build instructions are provided (again minor).

Tests with pytest ran successfully. The following output was produced, and the one warning could be suppressed/ignored if it's not an issue or addressed.

=============================================================== warnings summary ================================================================
../.conda/envs/pysm/lib/python3.9/site-packages/astropy/tests/helper.py:31
  /n/home11/smsharma/.conda/envs/pysm/lib/python3.9/site-packages/astropy/tests/helper.py:31: PytestUnknownMarkWarning: Unknown pytest.mark.remote_data - is this a typo?  You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/mark.html
    remote_data = pytest.mark.remote_data

-- Docs: https://docs.pytest.org/en/stable/warnings.html
============================================= 60 passed, 2 skipped, 1 warning in 195.64s (0:03:15) ==============================================

Running tests with tox initially failed, since I did not have an active MPI environment, and hence no mpi4py installed (I'm not sure why this wasn't an issue earlier). The specific error was:

fatal error: mpi.h: No such file or directory

It wasn't clear from the docs or README whether MPI was required or an optional enhancement to improve performance---this should be made clearer. If it's required, the fact that a working MPI installation is needed could be mentioned as well (since this seems to be the source of the error above). If not, the code and tests should be able to run without MPI.

After activating an appropriate compiler and MPI environment, the tox test completed successfully (modulo the warning from pytest above).

Functionality and API

After installation, I was able to run the examples successfully, including the Jupyter notebooks and pysm3_mpi.py with MPI. Although I wasn't able to test it out, it is clear from the examples that the functionality is easily extendable to custom use cases. The tight API integration with astropy is well done.

Just to conclude, most of the comments and suggestions above (with the exception of those re: MPI installation, testing, and usage perhaps) are very minor, and I've checked off the other items on the checklist.

zonca commented 3 years ago

thanks @smsharma! very good findings, See https://github.com/galsci/pysm/pull/93 for fixes on docs and test your warning was probably due to pytest-astropy missing, it should be fixed if you follow the new docs.

Next I'll look into MPI.

patricialarsen commented 3 years ago

Hi @zonca

I should have my review completed fully tomorrow. Just want to confirm that I can roughly replicate the performance numbers for the parallelization, and do another pass through the paper, but I don't expect issues to come up.

Overall I'm very happy with the code. As smsharma noted the API is very nice, and I've run through the tutorial examples and confirmed they're all working as expected (the existence of those is much appreciated), and I think the documentation between the model summaries, tutorial notebooks and paper are adequate for anyone using the code. I haven't tested every variation of model options, but between the tests and the tutorials I'm convinced that this should be fine.

A few minor concerns:

a) that the mpi section of the tutorial notebooks is much lighter than the rest:

It seems like the MPI compatibility is something that was really important in the development, so even though I guess it doesn't really work with the jupyter notebook format of the tutorials as well, it would still be really nice to have the syntax of the map distribution outlined in that tutorial section. Even just to copy that linked example into the text.

Then b) when I run the pysm3_mpi example I get a lot of warnings:

Most like the below:

python3.9/site-packages/pysm3/utils/data.py:42: UserWarning: Retrieve data for pysm_2/ame2_t_new.fits (if not cached already) warnings.warn(f"Retrieve data for {filename} (if not cached already)")

I believe that after posting this warning it does in fact retrieve the data, so can I suggest changing the warning from "retrieve data" to "retrieving data" to make that clear, and consider suppressing these since you get a little inundated?

There are other warnings like:

:228: RuntimeWarning: mpi4py.MPI.Comm size changed, may indicate binary incompatibility. Expected 32 from C header, got 40 from PyObject which are a bit more worrying. I think this is a compatibility issue with the default mpi4py version. These go away if I use mpi4py==3.0.3. This may just be my machine but I wonder if you could double check if a version requirement needs to be added for mpi4py? The new mpi versions were only released in August this year. c) do you also see in the classes list on the documentation one which looks like the below? PackageNotFoundError | The package was not found. (edit just noticed the CMB map class also doesn't seem to be linking to the description on the website docs properly) d) Finally I do find that although the run_mpi_tests.sh tests pass I get a lot of minor warnings (e.g. see below), it might be worth suppressing or fixing some of these /pysm3/models/template.py:221: AstropyDeprecationWarning: "verbose" was deprecated in version 1.15.0 and will be removed in a future version.
zonca commented 3 years ago

thanks @patricialarsen

A few minor concerns:

a) that the mpi section of the tutorial notebooks is much lighter than the rest:

It seems like the MPI compatibility is something that was really important in the development, so even though I guess it doesn't really work with the jupyter notebook format of the tutorials as well, it would still be really nice to have the syntax of the map distribution outlined in that tutorial section. Even just to copy that linked example into the text.

I am actually debating if I should remove the MPI functionality, it is used very rarely and makes the code significantly more complicated.

Then b) when I run the pysm3_mpi example I get a lot of warnings:

Most like the below:

python3.9/site-packages/pysm3/utils/data.py:42: UserWarning: Retrieve data for pysm_2/ame2_t_new.fits (if not cached already) warnings.warn(f"Retrieve data for {filename} (if not cached already)")

I think this is fixed in main, see https://github.com/galsci/pysm/pull/88, I will release a new version of PySM as soon as the review is finished, so all we worked on is going to be published.

I believe that after posting this warning it does in fact retrieve the data, so can I suggest changing the warning from "retrieve data" to "retrieving data" to make that clear, and consider suppressing these since you get a little inundated?

There are other warnings like: :228: RuntimeWarning: mpi4py.MPI.Comm size changed, may indicate binary incompatibility. Expected 32 from C header, got 40 from PyObject which are a bit more worrying.

I think this is a compatibility issue with the default mpi4py version. These go away if I use mpi4py==3.0.3. This may just be my machine but I wonder if you could double check if a version requirement needs to be added for mpi4py? The new mpi versions were only released in August this year.

I cannot reproduce this, I don't get warnings with mpi4py 3.1.1 Also I generally assume that people have installed also libsharp when they use MPI (I'll improve the docs explaining this) following https://pysm3.readthedocs.io/en/latest/#dependencies. Maybe that solves the issue?

c) do you also see in the classes list on the documentation one which looks like the below? PackageNotFoundError | The package was not found.

yes, I'll fix that.

(edit just noticed the CMB map class also doesn't seem to be linking to the description on the website docs properly)

do you mean from https://pysm3.readthedocs.io/en/latest/#classes?

d)

Finally I do find that although the run_mpi_tests.sh tests pass I get a lot of minor warnings (e.g. see below), it might be worth suppressing or fixing some of these

/pysm3/models/template.py:221: AstropyDeprecationWarning: "verbose" was deprecated in version 1.15.0 and will be removed in a future version.

this also should be fixed in main

zonca commented 3 years ago

@patricialarsen @smsharma ok, I merged https://github.com/galsci/pysm/pull/93, this has fixes to the warnings and an improved documentation about MPI

patricialarsen commented 3 years ago

@zonca

a) I'd leave it personally, I think it extends the use cases for the code. The improved documentation looks good to me. And the mpi functionality seems to be working.

b) Okay great, if it works on your end I'm happy to leave it. I had installed libsharp as suggested and am using python3.9, but it may just be my OS. If this comes up as an issue in future for some users I'd keep in mind that downgrading mpi4py might help.

c) Yes that's what I mean. The description is currently "Parameters:"

d) Great!

e) Final additional point: you updated the imports in the mpi example but seem to have missed this file: "pysm3_mpi_so_pysm_models.py".

patricialarsen commented 3 years ago

@christinahedges @zonca

I have completed the checklist and would be happy to pass this through review. The code is useful for the community, it is straightforward to install and use, the tests and advertised functionality all work and the issues we've come across are very minor (predominantly slight adjustments to documentation or warning messages) and have been dealt with or are being dealt with by the authors. The paper alongside this appears to meet the JOSS requirements.

I'd like to congratulate the authors on writing a very nice code, and thank them for their prompt responses to the comments. I hope the review process has proven helpful for them.

christinahedges commented 3 years ago

@whedon generate pdf

christinahedges commented 3 years ago

@whedon check references

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1086/427976 is OK
- 10.21105/joss.01298 is OK
- 10.1093/mnras/stx949 is OK
- 10.1088/0067-0049/208/2/20 is OK
- 10.1145/2833157.2833162 is OK
- 10.1051/0004-6361/201321494 is OK
- 10.1051/0004-6361/201322068 is OK
- 10.3847/1538-3881/aabc4f is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago

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

christinahedges commented 3 years ago

@zonca it seems like @patricialarsen and @smsharma have both agreed that your submission should be accepted! This is great news. I believe there are a few unchecked marks in the review, but that your new merged PR will solve these unchecked marks. Reading the thread, I believe the automated tests checkbox from @patricialarsen is unchecked only because of the warnings that occurred during execution.

It looks like you have no broken references in the paper. I find no significant copy editing necessary in your proof. I would suggest in the "Performance" section you might add a sentence explaining that the level to which the results of the PySM 2 and PySM3 models at Nside = 512 are identical, since I believe they should ideally produce close to identical results. This is only a gentle suggestion.

@zonca if you are happy and would like to make no further changes based on the review, please make a tagged release 🏷️ of the code (since the code and documentation has changed during the review) and archive it, and then please report the version number and archive DOI here in this thread. Please make sure this archived version has all the correct meta data (author names, title etc)!

smsharma commented 3 years ago

Thanks @zonca for taking the comments and suggestions into account, with their changes I'm happy to check off on the remaining items as well @christinahedges . Congrats to the authors on a very useful package!

zonca commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

zonca commented 3 years ago

thanks @christinahedges and @smsharma, you reported a lot of issues that nobody bothers to report so they never get fixed and make life for new users a lot worse! The review was very useful, thanks @patricialarsen.

Ok, I released 3.3.2 https://zenodo.org/record/5620869

christinahedges commented 3 years ago

@whedon set v3.3.2 as version

whedon commented 3 years ago

OK. v3.3.2 is the version.

christinahedges commented 3 years ago

@whedon set 10.5281/zenodo.5620868 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.5620868 is the archive.

christinahedges commented 3 years ago

@whedon recommend-accept

whedon commented 3 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1086/427976 is OK
- 10.21105/joss.01298 is OK
- 10.1093/mnras/stx949 is OK
- 10.1088/0067-0049/208/2/20 is OK
- 10.1145/2833157.2833162 is OK
- 10.1051/0004-6361/201321494 is OK
- 10.1051/0004-6361/201322068 is OK
- 10.3847/1538-3881/aabc4f is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago

:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/2719

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/2719, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
christinahedges commented 3 years ago

@smsharma @patricialarsen Thank you so much for your reviews! Based on your comments I've now recommended to the EIC that we accept this submission. Thanks again to everyone for their hard work.

danielskatz commented 3 years ago

@zonca - Please make sure this archived version has all the correct metadata (author names, title etc)! You can change zenodo metadata without changing the DOI...

danielskatz commented 3 years ago

In addition, I've suggested some changes to the paper in https://github.com/galsci/pysm/pull/94

And finally, many of your references don't have journals - this is because you are using astro-standards abbreviations for journals in some cases, which JOSS doesn't support. Please change things like journal = {\apj} to spell out the journal names instead.

zonca commented 3 years ago

@zonca - Please make sure this archived version has all the correct meta data (author names, title etc)! You can change zenodo metadata without changing the DOI...

thanks @danielskatz, yes, the title is just the package name with the version, authors are fine.

danielskatz commented 3 years ago

thanks @danielskatz, yes, the title is just the package name with the version, authors are fine.

yes, I realize this, and I am asking you to change the title of the archive in Zenodo to match the title of the paper

danielskatz commented 3 years ago

@whedon recommend-accept

generate a new proof for me to check...

whedon commented 3 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1086/427976 is OK
- 10.21105/joss.01298 is OK
- 10.1093/mnras/stx949 is OK
- 10.1088/0067-0049/208/2/20 is OK
- 10.1145/2833157.2833162 is OK
- 10.1051/0004-6361/201321494 is OK
- 10.1051/0004-6361/201322068 is OK
- 10.3847/1538-3881/aabc4f is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago

:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/2720

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/2720, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
danielskatz commented 3 years ago

@zonca - sorry, one more thing I missed - can you protect the Ps in Python in bib entries with {P} - in Thorne et al and Zonca et al

otherwise, this is ready to publish

zonca commented 3 years ago

@danielskatz never noticed before that double braces {{}} preserve capitalization... ;)

danielskatz commented 3 years ago

@whedon recommend-accept