Closed editorialbot closed 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
Software report:
github.com/AlDanial/cloc v 1.88 T=0.26 s (136.3 files/s, 69235.8 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 18 3141 3531 6701
Jupyter Notebook 5 0 2971 633
TeX 1 22 0 370
reStructuredText 6 81 68 104
Markdown 1 21 0 41
DOS Batch 1 8 1 26
YAML 2 4 10 26
make 1 4 7 9
-------------------------------------------------------------------------------
SUM: 35 3281 6588 7910
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repository
Wordcount for paper.md
is 835
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1093/mnras/stx804 is OK
- 10.3847/1538-4357/ac47fe is OK
- 10.1088/1538-3873/aaf5ad is OK
- 10.1051/0004-6361/201935470 is OK
- 10.1088/0004-637X/775/2/137 is OK
- 10.1093/mnras/stab1405 is OK
- 10.1088/0004-637X/802/2/107 is OK
- 10.3847/1538-4357/ac0252 is OK
- 10.3847/PSJ/ac3513 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1051/0004-6361/201322971 is OK
- 10.1038/nature23651 is OK
- 10.3847/2041-8213/aba9d3 is OK
- 10.3847/2041-8213/abd18e is OK
- 10.3847/0004-637X/820/1/78 is OK
- 10.1051/0004-6361/201834384 is OK
- 10.3847/2041-8213/ab8238 is OK
- 10.1051/0004-6361/202141943 is OK
MISSING DOIs
- None
INVALID DOIs
- None
@ideasrule, @LorenzoMugnai β This is the review thread for the paper. All of our communications 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#4873
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.
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Thanks for agreeing to review @ideasrule and @LorenzoMugnai!
I'm looking forward to your feedback.
Hello @MartianColonist. I have a couple of comments on the paper. The writing is clear and simple, but I feel more can be said about comparing to other software or, as it is called in the checklist, the "Field Status". You say something in the Statement of Need section, but it doesn't seem complete to me. Perhaps you can add a few words along the line of the conclusion of Barstow et al. 2022 (https://ui.adsabs.harvard.edu/abs/2022ExA....53..447B/abstract) to demonstrate that the performance of POISEIDON is comparable to others for 1D atmospheres. This will tell the reader that you not only provide 2D or 3D capabilities, but for the same exercises that your code performs as the others in the field. I think it might be nice to mention the Nemesis (https://www.sciencedirect.com/science/article/abs/pii/S0022407307003378?via%3Dihub) and Arcis (https://www.aanda.org/articles/aa/full_html/2020/10/aa37377-19/aa37377-19.html) software as well.
Hi @LorenzoMugnai,
Thanks for your comments, I'll let you know once the paper has been revised.
A couple quick answers / questions:
- There is a 1D model benchmark for POSEIDON in Appendix D of MacDonald & Lewis (2022) (https://ui.adsabs.harvard.edu/abs/2022ApJ...929...20M/abstract). The 1D model benchmark was 70 ms, which is a factor of a few faster than other transmission spectra codes I've tested against. I'll add these details to the paper.
That would be perfect, in my opinion.
- Could you clarify which conclusion of Barstow et al. 2022 you are referencing? POSEIDON was one of the codes included in that paper, so the results matched the other codes.
Exactly. My point is that you can cite that paper to show that POSEIDON results match the other codes for 1D atmosphere. This, I think, will contribute to comparing your software to others in the field and convince the reader about its robustness.
- A very good point about NEMESIS and ARCiS! I was just referencing open source retrieval codes (since there are around 20 other closed source codes mentioned in the literature). But it appears NEMESIS does have a GitHub repository, so I'll be sure to reference it. ARCiS is not open source from what I can tell.
You are right about ARCiS: I got confused by their web page.
@editorialbot commands
Hello @ideasrule, here are the things you can ask me to do:
# List all available commands
@editorialbot commands
# Get a list of all editors's GitHub handles
@editorialbot list editors
# Check the references of the paper for missing DOIs
@editorialbot check references
# Perform checks on the repository
@editorialbot check repository
# Adds a checklist for the reviewer using this command
@editorialbot generate my checklist
# Set a value for branch
@editorialbot set joss-paper as branch
# Generates the pdf paper
@editorialbot generate pdf
# Generates a LaTeX preprint file
@editorialbot generate preprint
# Get a link to the complete list of reviewers
@editorialbot list reviewers
Hi @LorenzoMugnai,
I've updated the paper to address your feedback. There is now a new paragraph at the end of the 'statement of need' section comparing POSEIDON to other codes for the 1D case.
Thanks for the suggestion!
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Hi @MartianColonist, I have read the new version of the manuscript and it seems more complete to me now. Thanks. I have updated my list accordingly.
Hello @MartianColonist, I am reading the documentation, and browsing your repository, but I can't find any "Automated tests" or "Community guidelines", which are in my check-list. Can you point me to the right direction?
Hi @LorenzoMugnai,
Regarding "Community guidelines", all there is at present is the brief 'support' section at the end of the GitHub README. I will add a dedicated section on community contribution guidelines to the documentation to improve this. I'll let you know once it's up.
For tests, it isn't obvious how to concoct future-proof automated tests for a radiative transfer and retrieval package like POSEIDON. For automated tests, two issues come to mind:
So I opted for a manual test approach: the tutorials in the documentation serve to verify POSEIDON's functionality. A user can download a Jupyter notebook for each tutorial in the guide by clicking the 'Download full notebook here' link at the top of each page. Since the expected output is included in the plots built into each Jupyter notebook, by successfully running the notebook cells the user can verify the functionality.
Per the JOSS review criteria, this is essentially:
"OK: Documented manual steps that can be followed to objectively check the expected functionality of the software (e.g., a sample input file to assert behavior)"
Unit testing really is tricky for retrieval codes! It's not mandated by JOSS, but I do recommend having some rudimentary unit tests, if only to catch obvious bugs and set up the framework for adding more tests later on. It'll give you some peace of mind when you make major revisions to the code.
Here are some suggestions for unit tests:
Your code supports 1D, 2D, and 3D atmospheres. Computing the transit spectrum for a 2D atmosphere with the inhomogeneity set to 0 should give you the same result as computing a 1D atmosphere, and similarly a 3D atmosphere with azimuthal symmetry should be the same as a 2D atmosphere. Is this the case?
There are some simple cases where you can analytically calculate the transit spectrum. For example, for an isothermal atmosphere with H << Rp and a cross section that doesn't depend on height, the transit depth is given by Equation 27 in Betremieux & Swain 2017: https://academic.oup.com/mnras/article/467/3/2834/2982874
I used it to compute the transit depth if a Rayleigh scattering haze were the only source of opacity (Eq 11-13 in this paper: https://arxiv.org/pdf/1811.11761.pdf)
Since the formula is linear in H and ln(abundance*sigma), it ought to be pretty easy to generalize to a temperature and/or ln(abundance) distribution that varies linearly with latitude.
You probably calculate the slant optical depth somewhere in the code. Does the slant optical depth tau_slant(P) equal tau_vert(P) sqrt(2piRs/H) = H n(P) sigma sqrt(2piRs/H), where n is the number density at pressure P, Rs is the planet radius at that pressure, sigma is the cross section, and H is the scale height? (This is all assuming an isothermal atmosphere where sigma is constant and H << Rs.) Also, if you take the slant optical depth that you calculate and apply the analytic formula (Betremieux & Swain 2017), do you get the transit depth that your code calculates?
It's very hard to test if a retrieval is giving you the right results, but you could put it in a unit test and force it to terminate after some small number of iterations, just to make sure the retrieval actually runs and doesn't crash immediately.
These are good suggestions, thanks @ideasrule!
I'll look into some unit tests along the lines above.
So far, I've successfully installed POSEIDON (a very painless process) and ran transmission_basic.ipynb and transmission_2D.ipynb without problem. However, transmission_terrestrial.ipynb runs into errors, possibly related to the open issue you have on Github:
Could you see what might be wrong?
I also had a quick comment on the paper. Looking at the line lists used to generate opacities, they mostly seem to be up to date and of high quality, but I notice that CH4 uses ExoMol's 34to10. This is good enough for low resolution work, but the line positions in this line list are not accurate enough for high resolution cross correlation spectroscopy. Would you consider switching to HITEMP 2020's line list? From their paper (https://iopscience.iop.org/article/10.3847/1538-4365/ab7a1a):
"The completeness of the 34to10 line list has improved when compared to 10to10, with the partitioning of the line list making it more practical to use. However, the underlying energy levels (and transition frequencies) have not been adjusted, and therefore, the accuracy issues noted for 10to10 remain relevant to 34to10. Line intensities are also significantly overestimated with respect to experimental data for high wavenumber ranges."
Hi @ideasrule,
The typo in 'transmission_terrestrial.ipynb' has been fixed on the main branch. Can you verify it works for you now?
Regarding the line lists, I'm part of a collaboration working on a NASA-supported opacity database (https://ui.adsabs.harvard.edu/abs/2022BAAS...54e.371B/abstract). Once this database is publicly released, I will comprehensively update all POSEIDON's cross sections (including CH4) to the latest line lists recommended by the ExoMol and HITRAN/HITEMP teams (who are both involved in this project). For the time being, the 34to10 CH4 cross section is more than sufficient for low spectral resolution JWST analyses (I don't find any significant differences in forward model comparisons vs. other codes). But everything will be updated when our new database is released.
I also note that since I am travelling for the next 3 weeks, I might be a little slow updating the code base in response to your feedback. Rest assured that I'm noting all this down, and I greatly appreciate all your feedback!
@MartianColonist I'm actually glad to hear that you're travelling, because I was worried that I was taking too long with the review :) Thanks for correcting the typo in transmission_terestrial.ipynb. I can verify that it works now, and so do the other notebooks.
To further test the code, I decided to set up a retrieval on MIRI/LRS data. I think the retrieval worked well, but the plotting ran into a minor issue: I tried to set wl_min to 4.5 and wl_max to 12.5, but the plot's x-axis is always 1, which squishes all of the useful range into the rightmost 40% of the plot. I got around this problem by adding "plt.xlim(wl_min, wl_max) right in front of plt.tight_layout() in visuals.py:plot_spectra_retrieved, but there's surely a better way to fix it.
Another minor issue: in retrieval_basic.ipynb, data_dir is 'Tutorial/WASP-999b', but the directory doesn't actually exist. I spent a while looking for it in order to see what the format of the data file is supposed to be, before looking at the source code and realizing it searches for that string and replaces it with a different path. Is it possible to give it the path directly? I suspect lots of people will try to use "Tutorial/WASP-999b" as an example data directory, just as I did, and get confused about why it doesn't exist.
Aside from the minor issues I mentioned in the last post, I have one major comment and one more minor issue.
Major comment: Could you indicate clearly in the article proof, the GitHub README, and the readthedocs home page what the state of support for emission spectroscopy is? The article proof only mentions transmission spectroscopy, the README doesn't mention either transmission or emission, and the readthedocs mentions only transmission in the first paragraph but has a section in the "Guide" on emission. Are emission forward modelling & retrieval official features of the package (which would mean it falls under this review), or are they beta features?
Minor issue: It would be good to have API documentation for the parameters in the visuals class methods, since almost every user will be using these methods routinely. I'm thinking particularly of plot_spectra_retrieved and plot_spectra, and particularly of parameters where the meaning isn't obvious (e.g. what are the options for y_unit)?
@ideasrule emission spectra are a beta feature added post JOSS submission while I was waiting for an editor to be assigned (and just pushed to main a few dyas ago). It is considered a beta feature because there is not currently a methods paper describing the forward model in the literature. I will add clear statements to the next paper and documentation update.
Yes, docstrings for visuals.py are on the to-do list. Thanks!
@ideasrule, @LorenzoMugnai, @MartianColonist β Happy new year! I'm writing to check in on the progress of this review. Please let me know if there are any major stoppers or if there's anything I can do to help move things along. Thanks!
@dfm Happy New Year! I was waiting for the author to make the updates he said he was going to make ("community guidelines", rudimentary "automated tests", a bit more documentation on the visuals class). Once those are done, I'm happy to put checkmarks on everything. In general, I've found POSEIDON to be a very well written, useful, and easy to use code, so good job to @MartianColonist !
Happy new year to everybody. @dfm , I am in the same situation as @ideasrule. I'm ready to give my approval. I was just waiting for the author to come back with the mentioned updates. Other than that, I haven't found any reason to stop posting. POSEIDON is a very nice product and deserves this publication.
A very happy new year to you all!
I have almost finished the requested updates (thanks for the great suggestions @ideasrule and @LorenzoMugnai!). I'll push the new version in the next day or so.
The one stumbling block I've encountered is with the automated tests. Since POSEIDON requires quite large input files to be downloaded (40+GB, dominated by the cross section database), GitHub Actions CI is proving problematic. The package can install correctly, but the automated download of the cross section database fails because Google Drive consistently blocks wget partway through the transfer of the largest file. Moving the files to Zenodo didn't help either, since downloads from Zenodo are sufficiently slow that GitHub Actions times out. I'd certainly appreciate any suggestions!
If there isn't an obvious way to get CI working, would it be ok to have the tests in the /tests folder without automation (such that a user can run them after installing the package)?
@MartianColonist β thanks for the update! Automation isn't strictly required for JOSS, but always good to have if you can. One option is to have a subset of tests that don't require the full download + some offline tests that aren't integrated into the CI system. But as I said this isn't formally required as far as I'm concerned.
The one stumbling block I've encountered is with the automated tests. Since POSEIDON requires quite large input files to be downloaded (40+GB, dominated by the cross section database), GitHub Actions CI is proving problematic. The package can install correctly, but the automated download of the cross section database fails because Google Drive consistently blocks wget partway through the transfer of the largest file. Moving the files to Zenodo didn't help either, since downloads from Zenodo are sufficiently slow that GitHub Actions times out. I'd certainly appreciate any suggestions!
Here's a suggestion: if you don't have tests that depend on the true cross sections, and I'm guessing you don't, what about making up the cross section files on the fly using a random number generator? Of course, it would be better for the order of magnitude to be approximately correct, not off by 10^10. This has the added advantage that if you make the cross sections the same at all altitudes (but not at all wavelengths), you can analytically calculate the transit spectrum and compare it to what your code outputs.
The new version is live!
Major changes:
Regarding the analytic test, here is POSEIDON compared to the analytic formula from Betremieux & Swain (2017):
The agreement is within 0.01 % (< 1 ppm).
The retrieval test takes about 1 minute to run and successfully recovers the true values of each parameter to < 0.5 %:
Thanks for your very helpful input @LorenzoMugnai, @ideasrule, and @dfm!
@editorialbot set main as branch
Done! branch is now main
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@editorialbot check references
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1093/mnras/stx804 is OK
- 10.3847/1538-4357/ac47fe is OK
- 10.1088/1538-3873/aaf5ad is OK
- 10.1051/0004-6361/201935470 is OK
- 10.1088/0004-637X/775/2/137 is OK
- 10.1093/mnras/stab1405 is OK
- 10.1088/0004-637X/802/2/107 is OK
- 10.3847/1538-4357/ac0252 is OK
- 10.3847/PSJ/ac3513 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1051/0004-6361/201322971 is OK
- 10.1038/nature23651 is OK
- 10.3847/2041-8213/aba9d3 is OK
- 10.3847/2041-8213/abd18e is OK
- 10.3847/0004-637X/820/1/78 is OK
- 10.1051/0004-6361/201834384 is OK
- 10.3847/2041-8213/ab8238 is OK
- 10.1051/0004-6361/202141943 is OK
- 10.1016/j.jqsrt.2007.11.006 is OK
- 10.3847/2041-8213/abb77f is OK
- 10.3847/1538-3881/ac0e99 is OK
- 10.1007/s10686-021-09821-w is OK
MISSING DOIs
- 10.5194/epsc2022-1114 may be a valid DOI for title: Ariel: Enabling planetary science across light-years
INVALID DOIs
- None
@editorialbot check repository
Software report:
github.com/AlDanial/cloc v 1.88 T=0.18 s (229.4 files/s, 120631.5 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 20 3436 4701 7241
Jupyter Notebook 7 0 4173 909
TeX 1 28 0 450
reStructuredText 7 118 92 160
YAML 3 16 10 98
Markdown 1 24 0 48
DOS Batch 1 8 1 26
make 1 4 7 9
-------------------------------------------------------------------------------
SUM: 41 3634 8984 8941
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repository
Wordcount for paper.md
is 1099
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
The changes look great, @MartianColonist ! I've checked off every item in the checklist. I only have three very minor suggestions left:
Your code's biggest selling point is that it supports 2D and 3D exoplanet atmospheres. I think you should mention this on the GitHub README.
For PLATON, the most up to date reference is Zhang et al 2020 (https://ui.adsabs.harvard.edu/abs/2020ApJ...899...27Z/abstract).
When I ran the references check, it reported that "Ariel: Enabling planetary science across light-years" is missing a DOI. You could use the arXiv DOI: https://doi.org/10.48550/arXiv.2104.04824
Great job developing POSEIDON!
Everything looks fine to me. I've checked all items on my list and I think the software is now ready to go. Great job, @MartianColonist , the community will benefit from your code!
@ideasrule, @LorenzoMugnai β Thanks for your thorough and constructive reviews!!
@MartianColonist β Things are looking good! 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. It would also be great if you can address the final suggestions from @ideasrule above.
Once you've done that:
@dfm, I've implemented the suggestions from @ideasrule and merged your pull request.
@editorialbot set 10.5281/zenodo.7530543 as archive
Submitting author: !--author-handle-->@MartianColonist<!--end-author-handle-- (Ryan J. MacDonald) Repository: https://github.com/MartianColonist/POSEIDON Branch with paper.md (empty if default branch): main Version: 1.0.0 Editor: !--editor-->@dfm<!--end-editor-- Reviewers: @ideasrule, @LorenzoMugnai Archive: 10.5281/zenodo.7530543
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
@ideasrule & @LorenzoMugnai, 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:
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 @LorenzoMugnai
π Checklist for @ideasrule