search

openjournals / joss-reviews

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

[REVIEW]: ExoRad 2.0: The generic point source radiometric model #5348

Closed editorialbot closed 1 year ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@LorenzoMugnai<!--end-author-handle-- (Lorenzo V. Mugnai) Repository: https://github.com/ExObsSim/ExoRad2-public Branch with paper.md (empty if default branch): joss Version: v2.1.121 Editor: !--editor-->@dfm<!--end-editor-- Reviewers: @skendrew, @eas342 Archive: 10.5281/zenodo.8367214

Status

status

Status badge code:

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

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

@skendrew & @eas342, 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 @dfm 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 @skendrew

📝 Checklist for @eas342

editorialbot commented 1 year 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 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1007/s10686-020-09676-7 is OK
- 10.1117/12.2629373 is OK
- 10.1007/s10686-020-09690-9 is OK
- 10.1088/1538-3873/aa65b0 is OK
- 10.1051/0004-6361/201425481 is OK
- 10.5194/epsc2022-618 is OK
- 10.1007/s10686-018-9611-4 is OK
- 10.1007/s11214-006-8315-7 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 1 year ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.15 s (879.5 files/s, 84722.4 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          60           1377           1576           6015
XML                              4             27            156            711
reStructuredText                56            397            343            695
Markdown                         3             86              0            329
TeX                              1             13              0            126
Jupyter Notebook                 1              0            429             95
YAML                             3             15             12             66
DOS Batch                        1              8              1             26
make                             1              4              7              9
-------------------------------------------------------------------------------
SUM:                           130           1927           2524           8072
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 1480

dfm commented 1 year ago

@skendrew, @eas342 — This is the review thread for the paper. All of our correspondence will happen here from now on. Thanks again for agreeing to participate!

👉 Please read the "Reviewer instructions & questions" in the first comment above, and generate your checklists by commenting @editorialbot generate my checklist on this issue ASAP. As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.

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#5348 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for the review process to be completed within about 4-6 weeks but please try to make a start 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. Please get your review started as soon as possible!

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:

LorenzoMugnai commented 1 year ago

Thank you for agreeing to review this work, @skendrew and @eas342 .

Looking forward to hearing your feedback!

eas342 commented 1 year ago

Review checklist for @eas342

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

skendrew commented 1 year ago

Review checklist for @skendrew

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

skendrew commented 1 year ago

is there a policy of all contributors being listed as co-author on the paper, or do we leave that up to the lead author/developer? a minor contribution was made by someone who I believe is not a co-author, not sure if that is something to raise?

dfm commented 1 year ago

@skendrew — good question! The JOSS authorship policy is here, and there isn't a requirement that all contributors are co-authors, or that all authors must have contributed to the codebase. We typically leave the specific authorship choices up to the authors and contributors unless there seems to be any significant omissions! It's common for minor contributions to be considered insufficient for co-authorship.

LorenzoMugnai commented 1 year ago

Hi. Thank you for noticing :) If you are referring to the contributors list in the repository, I believe that this is a very minor contribution. As you can see here, the contributor noticed that I left a directory with my name on it in the configuration file and suggested removing it. I would not consider it a contribution to the code.

dfm commented 1 year ago

@skendrew, @eas342 — Just checking in here to see how your reviews are going. Please let us know if you run into any issues or if there are any major stoppers! Thanks again for your work on this!!

eas342 commented 1 year ago

Sorry for the slow review and thanks for checking in. I do not see any major issues and hope to have my review finished up this week.

eas342 commented 1 year ago

Sorry I don't know the appropriate place to put comments on the text of the PDF so I'm putting them here @LorenzoMugnai.

eas342 commented 1 year ago

And the authors may consider citing other telescope simulation software

State of the field: Do the authors describe how this software compares to other commonly-used packages?

I'm honestly not too familiar with current telescope software that runs quickly like ExoRad but some examples of simulators I have seen are are Pandeia, WebbPSF, https://github.com/jarronl/pynrc , Code V, Zemax, Phosim (https://www.lsst.org/scientists/simulations/phosim).

Or maybe synphot is more similar to ExoRad

LorenzoMugnai commented 1 year ago

@eas342 Thanks for your comments! I am implementing them. In particular, I will add a paragraph on the comparison with other software. I'll let you know when it's ready. Thank you.

eas342 commented 1 year ago

I made a random list of 1000 stars, and confirmed the performance claim - it took 23 minutes to run on an M1mac.

eas342 commented 1 year ago

@LorenzoMugnai , the documentation looks good! I think the only thing that JOSS requires is a statement of need - I think that could be copied from the JOSS writeup.

LorenzoMugnai commented 1 year ago

@eas342 Thank you for your great feedback. I have updated the documentation, please let me know if it answers the requirements now:

A statement of need The authors should clearly state what problems the software is designed to solve, who the target audience is, and its relation to other work.

I am working on the paper too, but I am traveling and it is requiring me some more time. Thank again!

LorenzoMugnai commented 1 year ago

@editorialbot generate pdf

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:

LorenzoMugnai commented 1 year ago

And the authors may consider citing other telescope simulation software

State of the field: Do the authors describe how this software compares to other commonly-used packages?

I'm honestly not too familiar with current telescope software that runs quickly like ExoRad but some examples of simulators I have seen are are Pandeia, WebbPSF, https://github.com/jarronl/pynrc , Code V, Zemax, Phosim (https://www.lsst.org/scientists/simulations/phosim).

Or maybe synphot is more similar to ExoRad

Hi, @eas342 , I updated the manuscript by adding a paragraph (line 82). Please let me know if this is better. I only referred to Pandeia and synphot, because the others are more optical/PSF simulators, so are not really comparable to ExoRad. But I am open to any comments or suggestions.

I also updated the figures and the citation to the MULTIACCUM equation, as you suggested. Thank you

LorenzoMugnai commented 1 year ago

Hello @dfm and all, I trust you are well. I'm reaching out to inquire about the status of the ongoing review. Is there anything outstanding on my end that requires attention? Additionally, are there any significant issues detected that I need to resolve? Thank you once again for your work and help!!

dfm commented 1 year ago

@LorenzoMugnai — Thanks for checking in! I'll let @eas342 respond here, and I've pinged @skendrew via email with a reminder to check back in.

skendrew commented 1 year ago

I have mainly minor comments and apologies for being slow. Thank you for creating this software, having spent a lot of my career working in performance estimation of instruments at various stages of development I agree that having robust and versatile software for this work is very valuable. I like how an optical path can be entirely configured in the input.

I encountered no issues with the installation or functionality of the code. Documentation is well written and clear.

A few comments & questions for the text of the paper and/or documentation

Thanks!

eas342 commented 1 year ago

@LorenzoMugnai The new manuscript looks good - thank you for making the changes. I would just echo Sarah that it would be nice to verify that the results make sense on a calibrated telescope. Can you provide a simple example, say for HST, JWST, TESS etc. that can be checked against an Exposure Time Calculator or observed data? Other than that, I think it's good to go on my end.

dfm commented 1 year ago

Thanks @skendrew and @eas342 for your reviews and suggestions here!! I agree that adding a worked example as suggested to the documentation would increase the impact and usability of the code.

@LorenzoMugnai — Let us know if you have any questions with regards to these final suggestions, and let's get this over the finish line!

LorenzoMugnai commented 1 year ago

Thank you everybody for the great feedbacks! I am traveling in these weeks and I have limited connection. But I am working in answering your comments with my coauthors. I will be back to you with an amended version of the manuscript (and of the documentation) as soon as possible. Thank you all again!

LorenzoMugnai commented 1 year ago

Hi all, sorry for the late reply. I just updated the manuscript, and I will ask the bot to compile it in a minute, but first I will try now to address all your comments.

- I think it would be very useful if you could include in the text of the paper a short discussion of the use of the code for ground-based vs. space-based instruments. Or perhaps include an example in the code for a ground-based telescope/instrument? I gather it's been written primarily for space-based (or airborne) systems, are there limitations to its use for ground-based systems?

We are not aware of any limitations for a ground-based system. We didn't have the chance to apply the simulator to such a use case yet, but given the possibility to add multiple foregrounds, such as the atmospheric emission and transmission, we don't foresee any problem in implementing a ground-based telescope on ExoRad. We now mention it in the manuscript.

- I can see in the payload_example input file that there are several options for providing a PSF. The description in the paper and documentation of this is fairly sparse. Can you provide a bit more information on what options are available for providing custom PSFs? Are time-varying PSFs supported?

You are right: we didn't document properly the PSF module. We now included a dedicated section in the docs. However, time-varying PSFs are not supported by this code, as ExoRad is a Radiometric simulator and it cannot support time-dependent objects.

- has the code been validated against any actual working telescopes/instruments? you mention validation against ArielRad and ExoSim; can you elaborate in the paper on how these have been validated against working systems? this would be useful for the reader.

Sure, ExoSim has been validated against HST and JWST, and therefore our validation against ExoSim code is validation against these systems.
However, in the paper, we now mention the validation against Pandexo (which is validation against JWST). We prefer this example because the Pandexo software is widely known and it is more similar to ExoRad than ExoSim and therefore is a like-to-like comparison. We didn't include a full example with all the configuration file in the documentation because a dedicated version of ExoRad for the JWST (which will support different observing modes) is under preparation, and we plan to present it with a dedicated publication.

- it would be good to clarify what the min/max wavelengths are that the code can handle. I can see that the user can specify cutoff wavelengths, but presumably, there is some limit.

From a software perspective, we are not aware of any limitation on wavelength range: ExoRad functions should be able to handle any wavelength range. Cutoff wavelengths are related to detectors: different detectors have different responsivities, which can be described by the quantum efficiencies data and by the detector cutoff. So, the only limits we can think about are related to the physics, and not to the software. For example, if we want to simulate a microwave or an X-ray observation, we might need to update the code with a different instrument than a photometer and spectrometer, because the detectors used are different. This however is not, in our opinion, a software limitation, but a use case.

Please, let me know if you have any more questions or if my answers are not enough. Thanks again for your help.

LorenzoMugnai commented 1 year ago

@editorialbot generate pdf

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:

LorenzoMugnai commented 1 year ago

Hi @dfm and team,

Hope everything's going well on your end. Just checking in about the current review status. Do you need anything further from me? Are my recent answers and modifications satisfactory?

Thanks again for all your efforts and assistance!

dfm commented 1 year ago

Thanks for checking in @LorenzoMugnai!

Pinging @skendrew and @eas342 to take a last look to see if you can finish your checklists at this point. Please let @LorenzoMugnai know if you have have any further suggestions!

eas342 commented 1 year ago

Sorry for the slow replies here @LorenzoMugnai and @dfm. It looks good and my checklist is now complete.

LorenzoMugnai commented 1 year ago

@dfm and @eas342 Thanks for the feedback you've shared so far. I've been going over everything and noticed there's still an open item on @skendrew's checklist. Are there any concerns or issues with the automated tests I've set up? If there are elements missing or ways I can enhance them, please let me know. Always aiming to make things as smooth as possible!

dfm commented 1 year ago

@LorenzoMugnai — thanks! I also noticed how close this review is, add reached out to @skendrew via email next week. I expect we can get everything wrapped up soon! I'm about to go offline for 1.5 weeks, but I'll check back in as soon as I'm back online.

skendrew commented 1 year ago

I am happy to sign off on this - checklist complete.

dfm commented 1 year ago

@skendrew, @eas342 — Thanks for your thorough and constructive reviews!!

@LorenzoMugnai — I've opened a small PR with some minor edits to the manuscript, please take a look and merge or let me know what you think.

Once you've done that:

  1. Take one last read through the manuscript to make sure that you're happy with it (it's harder to make changes later!), especially the author names and affiliations. I've taken a pass and it looks good to me!
  2. Increment the version number of the software and report that version number back here.
  3. Create an archived release of that version of the software (using Zenodo or something similar). Please make sure that the metadata (title and author list) exactly match the paper. Then report the DOI of the release back to this thread.
LorenzoMugnai commented 1 year ago

Hello everybody, thank you very much for your work! I really appreciated your feedbacks.

@dfm I merged your request and followed your instructions:

  1. looks good to me
  2. v2.1.121(https://github.com/ExObsSim/ExoRad2-public/releases/tag/v2.1.121)
  3. 10.5281/zenodo.8367214 (https://zenodo.org/record/8367214)
dfm commented 1 year ago

@editorialbot set v2.1.121 as version

editorialbot commented 1 year ago

Done! version is now v2.1.121

dfm commented 1 year ago

@editorialbot set 10.5281/zenodo.8367214 as archive

editorialbot commented 1 year ago

Done! archive is now 10.5281/zenodo.8367214

dfm commented 1 year ago

@editorialbot generate pdf

dfm 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.1007/s10686-020-09676-7 is OK
- 10.1117/12.2629373 is OK
- 10.1007/s10686-020-09690-9 is OK
- 10.1088/1538-3873/aa65b0 is OK
- 10.1051/0004-6361/201425481 is OK
- 10.5194/epsc2022-618 is OK
- 10.1117/12.2641373 is OK
- 10.1007/s11214-006-8315-7 is OK
- 10.1086/520887 is OK
- 10.1117/12.2231768 is OK

MISSING DOIs

- None

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:

dfm commented 1 year ago

@editorialbot recommend-accept

editorialbot commented 1 year ago 
Attempting dry run of processing paper acceptance...