-
-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REVIEW]: The Python Sky Model 3 software #3783
Comments
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 🎉. 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. ⭐ Important ⭐ 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:
For a list of things I can do to help you, just type:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
Wordcount for |
|
|
@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 #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. |
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 It works okay if I change the flag in line 45 of setup.cfg from 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 The dependencies I had to add to the deps section of tox.ini are: I then manage to pass tests using tox with A large number of warnings, which mostly look like 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? |
We do not have any code block in the documentation See openjournals/joss-reviews#3783 (comment)
thanks @patricialarsen I started to work on your recommendations. About I tried with a plain conda environment with only python 3.8.
and it worked fine. How are you testing it? |
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. |
👋 @smsharma, please update us on how your review is going (this is an automated reminder). |
👋 @patricialarsen, please update us on how your review is going (this is an automated reminder). |
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. |
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. |
@christinahedges sorry for the delay, I'll get my next round of comments in before the weekend |
Hi! Congrats to the authors and contributors first of all for a very nice package that's extremely useful to the CMB community. InstallationI 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. TestsI had to install Building the docs locally required installing Tests with =============================================================== 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
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 Functionality and APIAfter installation, I was able to run the examples successfully, including the Jupyter notebooks and 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. |
thanks @smsharma! very good findings, Next I'll look into MPI. |
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) 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: 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) (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. |
thanks @patricialarsen
I am actually debating if I should remove the MPI functionality, it is used very rarely and makes the code significantly more complicated.
I think this is fixed in
I cannot reproduce this, I don't get warnings with mpi4py 3.1.1
yes, I'll fix that.
do you mean from https://pysm3.readthedocs.io/en/latest/#classes?
this also should be fixed in main |
@patricialarsen @smsharma ok, I merged galsci/pysm#93, this has fixes to the warnings and an improved documentation about MPI |
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉 openjournals/joss-papers#2720 If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#2720, then you can now move forward with accepting the submission by compiling again with the flag
|
@zonca - sorry, one more thing I missed - can you protect the otherwise, this is ready to publish |
@danielskatz never noticed before that double braces |
@whedon recommend-accept |
|
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉 openjournals/joss-papers#2721 If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#2721, then you can now move forward with accepting the submission by compiling again with the flag
|
@whedon accept deposit=true |
|
🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦 |
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨 Here's what you must now do:
Any issues? Notify your editorial technical team... |
Congratulations to @zonca (Andrea Zonca) and co-authors!! And thanks to @christinahedges for editing, and @patricialarsen and @smsharma for reviewing! |
🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉 If you would like to include a link to your paper from your README use the following code snippets:
This is how it will look in your documentation: We need your help! Journal of Open Source Software 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:
|
✨✨✨ Congrats on your first paper as editor @christinahedges ✨ ✨✨ |
We do not have any code block in the documentation See openjournals/joss-reviews#3783 (comment)
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
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 badge code:
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:
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
The text was updated successfully, but these errors were encountered: