openjournals / joss-reviews

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

[REVIEW]: PACMAN: A pipeline to reduce and analyze Hubble Wide Field Camera 3 IF Grism data #4838

Closed editorialbot closed 1 year ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@sebastian-zieba<!--end-author-handle-- (Sebastian Zieba) Repository: https://github.com/sebastian-zieba/PACMAN Branch with paper.md (empty if default branch): Version: v0.3 Editor: !--editor-->@xuanxu<!--end-editor-- Reviewers: @astrobel, @aureliocarnero Archive: 10.5281/zenodo.7465579

Status

status

Status badge code:

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

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

@astrobel & @aureliocarnero, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

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

Checklists

📝 Checklist for @astrobel

📝 Checklist for @aureliocarnero

editorialbot commented 2 years ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

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

@editorialbot commands

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

@editorialbot generate pdf
editorialbot commented 2 years ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.23 s (407.7 files/s, 67879.9 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          60           1793           3336           5550
reStructuredText                20            857            559            985
TeX                              1             41              0            475
Markdown                         3            100              0            215
YAML                             4             15              6             79
Jupyter Notebook                 2              0           1352             62
DOS Batch                        1              8              1             26
make                             1              4              7              9
TOML                             1              0              0              3
-------------------------------------------------------------------------------
SUM:                            93           2818           5261           7404
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago

Wordcount for paper.md is 1531

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

OK DOIs

- 10.1038/nature12888 is OK
- 10.3847/1538-3881/aac3df is OK
- 10.1086/683602 is OK
- 10.1086/131801 is OK
- 10.1086/670067 is OK
- 10.3847/1538-3881/aa6481 is OK
- 10.3847/0004-637X/832/2/202 is OK
- 10.1093/mnras/staa278 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1051/0004-6361/201322068 is OK
- 10.3847/1538-3881/aabc4f is OK
- 10.21105/joss.00024 is OK
- 10.1086/338770 is OK
- 10.1088/2041-8205/793/2/L27 is OK
- 10.1088/0004-637X/774/2/95 is OK
- 10.3847/2041-8205/822/1/L4 is OK
- 10.1038/nature13785 is OK
- 10.1093/mnras/stt1243 is OK
- 10.1038/s41586-018-0067-5 is OK
- 10.3847/1538-3881/abf3c3 is OK
- 10.1051/0004-6361/202038620 is OK
- 10.1117/12.789372 is OK
- 10.1126/sciadv.aav1784 is OK
- 10.3847/2041-8213/ab20c8 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 2 years ago

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

astrobel commented 2 years ago

Review checklist for @astrobel

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

astrobel commented 2 years ago

Hi - review in progress! As I work through the Quickstart tutorial, I've opened Issue sebastian-zieba/PACMAN#6.

astrobel commented 2 years ago

Hi @sebastian-zieba, thanks for the great software package! PACMAN is intuitive for the new user and covers a lot of ground really efficiently, and on these grounds I'm happy to recommend it for publication in JOSS. I have several notes below which I hope can help improve the documentation and the example case in particular.

General notes

workdir:  ./.ipynb_checkpoints/
eventlabel:
Traceback (most recent call last):
  File "/home/isy/aur/PACMAN/src/pacman/pacman_script.py", line 135, in <module>
    main()
  File "/home/isy/aur/PACMAN/src/pacman/pacman_script.py", line 106, in main
    update_meta(eventlabel, workdir)
  File "/home/isy/aur/PACMAN/src/pacman/lib/update_meta.py", line 14, in update_meta
    meta = me.loadevent(workdir + '/WFC3_' + eventlabel + "_Meta_Save")
  File "/home/isy/aur/PACMAN/src/pacman/lib/manageevent.py", line 143, in loadevent
    handle = open(filename + '.dat', 'rb')
FileNotFoundError: [Errno 2] No such file or directory: './.ipynb_checkpoints//WFC3__Meta_Save.dat'
background_box               False
bg_rmin                      100
bg_rmax                      400
bg_cmin                      40
bg_cmax                      100

correct_wave_shift_refspec   True
remove_which_orb            [0]

ld_model                     1

Documentation

Paper

The paper is great, and very thorough! I only have a couple of notes:

sebastian-zieba commented 2 years ago

Hi @astrobel, thank you for the review and the positive feedback!! I will work on your comments in the next few days and keep you updated on my progress.

sebastian-zieba commented 2 years ago

Hi @astrobel,

thanks again for your comments! I’m still working on the JOSS paper, but I’ve already incorporated all of the other code or documentation-related comments. Here are my replies to your individual comments:

General notes

  • I'm confused by the pip install .[test] line and instructions in the installation section of the docs. Nevertheless, by installing pytest in my conda environment I'm able to run the tests, which pass with 36 warnings. I don't think these warnings need to be addressed for acceptance to JOSS, but I'd be happy to send them your way if you're interested.

The [test] argument will additionally install the testing requirements (e.g., pytest). If you just do pip install -e . it will install the necessary PACMAN requirements (like astropy, numpy, …) but not pytest. I added more explanation on the [test] (and the [docs]) arguments in the installing instructions.

Here’s the text i added to https://pacmandocs.readthedocs.io/en/latest/installation.html:

.. note:: pip install -e . will only install the necessary dependencies. If you also want to install the dependencies to run the tests (using pytest) or work on the docs, you have to use the [test] or [docs] arguments respectively after pip install -e .. See Test Your Installation <https://pacmandocs.readthedocs.io/en/latest/installation.html#test-your-installation>_ for an example.

I also added the following further down:

.. code-block:: console

pip install .[test]

The [test] argument will also install the necessary dependencies to run pytest.

Regarding the tests: I am also getting some warnings when running the tests. I will work on these in the upcoming releases. Thanks for pointing this out.

  • If running Stage 01 in a directory that contains a Jupyter Notebook file & associated directory, .ipynb_checkpoints, Stage 01 detects that as the run directory and exits with the following output:
workdir:  ./.ipynb_checkpoints/
eventlabel:
Traceback (most recent call last):
  File "/home/isy/aur/PACMAN/src/pacman/pacman_script.py", line 135, in <module>
    main()
  File "/home/isy/aur/PACMAN/src/pacman/pacman_script.py", line 106, in main
    update_meta(eventlabel, workdir)
  File "/home/isy/aur/PACMAN/src/pacman/lib/update_meta.py", line 14, in update_meta
    meta = me.loadevent(workdir + '/WFC3_' + eventlabel + "_Meta_Save")
  File "/home/isy/aur/PACMAN/src/pacman/lib/manageevent.py", line 143, in loadevent
    handle = open(filename + '.dat', 'rb')
FileNotFoundError: [Errno 2] No such file or directory: './.ipynb_checkpoints//WFC3__Meta_Save.dat'
  • I was able to fix this by moving to a clean directory. As it was an easy fix, I haven't opened an issue, but let me know if you want one opened for it!

That’s a great point, thanks! I changed the pacman_script.py file. Stage 01 (and the later stages) will therefore ignore directories in the run directory that don't have names starting with "run_".

  • In the Quickstart tutorial, the following parameters are missing from Stage 20 in the example PCF:
background_box               False
bg_rmin                      100
bg_rmax                      400
bg_cmin                      40
bg_cmax                      100

correct_wave_shift_refspec   True

Fixed

  • Also, the PCF default for Stage 20 doesn't save any of the figures from the Quickstart example, I think it's worth changing that or at least highlighting the relevant PCF lines in the docs.

Fixed

  • And in Stage 30, the example PCF is missing the following:
remove_which_orb            [0]

ld_model                     1

Fixed

  • Please include community guidelines for third parties wishing to contribute to the software, preferably in the readme on the PACMAN landing page!

I added some information on how to contribute to the README on GitHub.

Documentation

  • The links to the individual stages on the "Stages" page are broken.

Thanks!! I actually removed the links there. They were links to the source code of the individual stages. I think that makes it more confusing and the user can instead just check out the code on GitHub (if they want).

  • Under "Repository Structure", it might be worth noting that the user should move the run files to their working directory. To keep things clear and separate, I suggest not including lines of example code in the 'Repository Structure" section, and keeping all code examples to the Quickstart section. Where necessary, you can point a link to a relevant section of the tutorial from the structural sections of the documentation.

Thanks! I agree. I removed the code in Repository Structure and noted clearly now that the user has to copy the contents of run_files into the work directory.

  • In the quickstart tutorial, setting up the directory structure should be included before the download instructions, as that's foundational to how PACMAN works. I strongly suggest incorporating a step that downloads/moves the ima files directly into the user's "data" directory.

Yes! I worked on making the whole Before Running and Stage 00 Section clearer. I have the following order for the Quickstart now: Introduction to Quickstart -> Setting up directories (data and run directory) -> download data and move to data directory -> start with stage 00.

I'll let you know when I also pushed the changes related to the paper.

xuanxu commented 2 years ago

@grburgess Please update us on how your review is going. You can create your checklist with the following command: @editorialbot generate my checklist

grburgess commented 2 years ago

@xuanxu I am a bit behind on ref reports, but will get to it this week. Apologies

sebastian-zieba commented 2 years ago

Hi @astrobel ,

I just pushed the last changes related to the paper. I have therefore now addressed all your comments and incorporated your suggested changes to the code, documentation and paper.

Here are my changes to the paper:

  • Please clarify that it's GJ1214 shown in the example Figures 1 and 2. Are there other papers on this object or are these new results? This will also help clarify the reproducability of these results.

I updated this part and clearly state now that the plots I am showing in the paper only use three Hubble-visits of the planet GJ 1214 b. I reference the paper Kreidberg+2014 which analyzed all 15 visits. The plots I am showing are therefore not new results but a reanalysis of public data. The results using PACMAN are also consistent with the published results.

(I also updated the figures 1 & 2 to make them a bit prettier.)

  • I think you could flesh out the state of the field a bit more. Are there other commonly-used pipelines that perform parts of what PACMAN can accomplish all-in-one? This could be included in the statement of need section where you mention that other pipelines have produced discrepant results.

I also updated the statement of need listing some examples by how PACMAN sets itself apart from other codes.

Best, Sebastian

xuanxu commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

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

astrobel commented 2 years ago

Hi @sebastian-zieba, thanks so much for all your work!! I've gone through the changes and I'm now satisfied that you've addressed my concerns, and I'm happy to confirm (cc @xuanxu!) that I've completed the checklist and I believe the work is publication standard.

xuanxu commented 2 years ago

@editorialbot check references

xuanxu commented 2 years ago

@editorialbot generate pdf

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

OK DOIs

- 10.1038/nature12888 is OK
- 10.3847/1538-3881/aac3df is OK
- 10.1086/683602 is OK
- 10.1086/131801 is OK
- 10.1086/670067 is OK
- 10.3847/1538-3881/aa6481 is OK
- 10.3847/0004-637X/832/2/202 is OK
- 10.1093/mnras/staa278 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1051/0004-6361/201322068 is OK
- 10.3847/1538-3881/aabc4f is OK
- 10.21105/joss.00024 is OK
- 10.1086/338770 is OK
- 10.1088/2041-8205/793/2/L27 is OK
- 10.1088/0004-637X/774/2/95 is OK
- 10.3847/2041-8205/822/1/L4 is OK
- 10.1038/nature13785 is OK
- 10.1093/mnras/stt1243 is OK
- 10.1038/s41586-018-0067-5 is OK
- 10.3847/1538-3881/abf3c3 is OK
- 10.1051/0004-6361/202038620 is OK
- 10.1117/12.789372 is OK
- 10.1126/sciadv.aav1784 is OK
- 10.3847/2041-8213/ab20c8 is OK
- 10.3847/1538-3881/aae8e5 is OK
- 10.3847/1538-4365/abe70e is OK
- 10.5281/zenodo.1998447 is OK
- 10.21105/joss.03285 is OK
- 10.1093/mnras/stz2688 is OK

MISSING DOIs

- Errored finding suggestions for "Infrared Spectroscopy of the Transiting Exoplanets...", please try later
- 10.3847/1538-4365/abe70e may be a valid DOI for title: allesfitter: Flexible star and exoplanet inference from photometry and radial velocity

INVALID DOIs

- None
editorialbot commented 2 years ago

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

xuanxu commented 1 year ago

@grburgess Friendly reminder. Please create your checklist with the following command: @editorialbot generate my checklist

xuanxu commented 1 year ago

@grburgess Please update us on your availablity to complete the review.

xuanxu commented 1 year ago

@editorialbot remove @grburgess from reviewers

J. Michael has not started the review yet and has been unresponsive here and by email for weeks so I'm switching to another reviewer.

editorialbot commented 1 year ago

@grburgess removed from the reviewers list!

xuanxu commented 1 year ago

@editorialbot add @aureliocarnero as reviewer

editorialbot commented 1 year ago

@aureliocarnero added to the reviewers list!

xuanxu commented 1 year ago

Hey @aureliocarnero: thanks for agreeing to review!

As a first step, please create your checklist with the following command: @editorialbot generate my checklist

aureliocarnero commented 1 year ago

Review checklist for @aureliocarnero

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

aureliocarnero commented 1 year ago

Congratulations to the author. Thank you to the first reviewer as well, their recommendations were very good and now the product is ready for submission at JOSS, meeting all the mandatory items and desired standards. (cc @xuanxu!)

xuanxu commented 1 year ago

Thanks @astrobel & @aureliocarnero for the reviews!

xuanxu commented 1 year ago

@editorialbot generate pdf

xuanxu commented 1 year ago

@editorialbot check references

editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1038/nature12888 is OK
- 10.3847/1538-3881/aac3df is OK
- 10.1086/683602 is OK
- 10.1086/131801 is OK
- 10.1086/670067 is OK
- 10.3847/1538-3881/aa6481 is OK
- 10.3847/0004-637X/832/2/202 is OK
- 10.1093/mnras/staa278 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1051/0004-6361/201322068 is OK
- 10.3847/1538-3881/aabc4f is OK
- 10.21105/joss.00024 is OK
- 10.1086/338770 is OK
- 10.1088/2041-8205/793/2/L27 is OK
- 10.1088/0004-637X/774/2/95 is OK
- 10.3847/2041-8205/822/1/L4 is OK
- 10.1038/nature13785 is OK
- 10.1093/mnras/stt1243 is OK
- 10.1038/s41586-018-0067-5 is OK
- 10.3847/1538-3881/abf3c3 is OK
- 10.1051/0004-6361/202038620 is OK
- 10.1117/12.789372 is OK
- 10.1126/sciadv.aav1784 is OK
- 10.3847/2041-8213/ab20c8 is OK
- 10.3847/1538-3881/aae8e5 is OK
- 10.3847/1538-4365/abe70e is OK
- 10.5281/zenodo.1998447 is OK
- 10.21105/joss.03285 is OK
- 10.1093/mnras/stz2688 is OK

MISSING DOIs

- 10.3847/1538-4365/abe70e may be a valid DOI for title: allesfitter: Flexible star and exoplanet inference from photometry and radial velocity

INVALID DOIs

- None
editorialbot commented 1 year ago

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

xuanxu commented 1 year ago

OK @sebastian-zieba, everything looks good, here are the next steps:

Once you do that please report here the version number and archive DOI

xuanxu commented 1 year ago

@sebastian-zieba please ping me here when you have the new version & archive DOI ready

sebastian-zieba commented 1 year ago

@xuanxu Thanks for your work! I created a new tag on GitHub (v0.3). The new Zenodo DOI is 10.5281/zenodo.7465579 I checked that the authors on the JOSS paper are the same as the ones listed on Zenodo. I also added the ORCIDs to the Zenodo release.

sebastian-zieba commented 1 year ago

I did also notice a typo in the title of the paper. I corrected it in the new tagged version (v0.3) (the one I'm referring to in the earlier post). (Instead of "...Camera 3 IF Grism data" it's now "...Camera 3 IR Grism data"). I just wanted to point it out, to make sure it's correct in the final version.

xuanxu commented 1 year ago

Thanks @sebastian-zieba!

xuanxu commented 1 year ago

@editorialbot generate pdf

xuanxu commented 1 year ago

@editorialbot set v0.3 as version

editorialbot commented 1 year ago

Done! version is now v0.3

editorialbot commented 1 year ago

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

xuanxu commented 1 year ago

@editorialbot set 10.5281/zenodo.7465579 as archive

editorialbot commented 1 year ago

Done! Archive is now 10.5281/zenodo.7465579

xuanxu commented 1 year ago

Looking good!

xuanxu commented 1 year ago

@editorialbot recommend-accept

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

OK DOIs

- 10.1038/nature12888 is OK
- 10.3847/1538-3881/aac3df is OK
- 10.1086/683602 is OK
- 10.1086/131801 is OK
- 10.1086/670067 is OK
- 10.3847/1538-3881/aa6481 is OK
- 10.3847/0004-637X/832/2/202 is OK
- 10.1093/mnras/staa278 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1051/0004-6361/201322068 is OK
- 10.3847/1538-3881/aabc4f is OK
- 10.21105/joss.00024 is OK
- 10.1086/338770 is OK
- 10.1088/2041-8205/793/2/L27 is OK
- 10.1088/0004-637X/774/2/95 is OK
- 10.3847/2041-8205/822/1/L4 is OK
- 10.1038/nature13785 is OK
- 10.1093/mnras/stt1243 is OK
- 10.1038/s41586-018-0067-5 is OK
- 10.3847/1538-3881/abf3c3 is OK
- 10.1051/0004-6361/202038620 is OK
- 10.1117/12.789372 is OK
- 10.1126/sciadv.aav1784 is OK
- 10.3847/2041-8213/ab20c8 is OK
- 10.3847/1538-3881/aae8e5 is OK
- 10.3847/1538-4365/abe70e is OK
- 10.5281/zenodo.1998447 is OK
- 10.21105/joss.03285 is OK
- 10.1093/mnras/stz2688 is OK

MISSING DOIs

- 10.3847/1538-4365/abe70e may be a valid DOI for title: allesfitter: Flexible star and exoplanet inference from photometry and radial velocity

INVALID DOIs

- None
editorialbot commented 1 year ago

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

Check final proof :point_right::page_facing_up: Download article

If the paper PDF and the deposit XML files look good in https://github.com/openjournals/joss-papers/pull/3838, then you can now move forward with accepting the submission by compiling again with the command @editorialbot accept