Open editorialbot opened 1 month ago
editorialbot
commented
1 month 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 commandsFor example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdfReference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.3847/1538-3881/ad2c8b is OK
- 10.3847/1538-3881/aaffd3 is OK
- 10.21105/joss.00024 is OK
- 10.1093/mnras/staa228 is OK
- 10.1051/0004-6361/200913396 is OK
- 10.1145/368996.369025 is OK
- 10.1051/0004-6361/201935470 is OK
- 10.21105/joss.05875 is OK
- 10.1111/j.1365-2966.2004.08585.x is OK
MISSING DOIs
- None
INVALID DOIs
- None
Software report:
github.com/AlDanial/cloc v 1.90 T=0.31 s (404.5 files/s, 212328.2 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 64 6016 6248 23785
Fortran 90 4 1002 422 3671
Jupyter Notebook 16 0 10990 3307
HTML 1 7 3 2763
XML 2 0 20 2281
JSON 7 0 0 1119
TeX 4 114 37 1103
reStructuredText 9 391 1164 695
Markdown 3 84 0 513
Meson 10 13 9 143
TOML 1 5 2 90
make 2 14 13 56
SQL 1 0 0 24
YAML 1 4 11 10
SVG 1 0 1 3
-------------------------------------------------------------------------------
SUM: 126 7650 18920 39563
-------------------------------------------------------------------------------Commit count by author:
686 Doriann Blain
340 Evert Nasedkin
325 mauricemolli
201 dblain
121 Paul Mollière
103 Nicholas Evert Nasedkin
7 Tomas Stolker
6 Paul Molliere
2 System Administrator
1 samderegtPaper file info:
📄 Wordcount for paper.md is 2121
✅ The paper includes a Statement of need section
editorialbot
commented
1 month ago License info:
✅ License found: MIT License (Valid open source OSI approved license)
editorialbot
commented
1 month ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
mbobra
commented
1 month ago @spectralcode, @benhord Thank you so much for agreeing to review! You can find the article in the comment boxes above ⬆️ , the software repository linked in the first comment box on this issue. To generate your checklist, use the following command:
@editorialbot generate my checklistPlease complete your review within 10 weeks, by October 1.
Again, JOSS is an open review process and we encourage communication between the reviewers, the submitting author, and the editor. And please feel free to ask me questions, I'm always around.
Can you please respond here (or give a thumbs up) so I know you're in the right place and found all the materials?
spectralcode
commented
1 month ago 
benhord
commented
1 month ago spectralcode
commented
1 month ago Hi @doriannblain
for transparency, I should note that the subject of your submission is outside of my field of research. I am able to perform the review according to the JOSS review guidelines and criteria, but I may not be able to provide much insights beyond that.
I was able to replicate the results from your Getting Started tutorial and SpectralModel tutorial. Besides minor issues related to slow download speeds of opacity files with Jupyter Lab (see reported issue in GitLab repository) everything worked fine. The documentation is well written and easy to follow. Good job!
In the documentation, you mention that there is currently no explanation for the model parameters of the SpectralModel yet. I believe that even a very rudimentary explanation for all the parameters, along with information about the expected units of the parameter values, would be very helpful.
There are already two publications regarding petitRADTRANS. The focus of this new submission is the new SpectralModel object within petitRADTRANS, which makes modeling and retrieval of high-resolution spectra easier. As I understand it, the main scholarly work is spectral_model.py and the corresponding documentation. Is that correct?
Is the method or code that was used to generate the results of Table 1 in the paper available? I would like to see if I can obtain similar values for memory and speed on a comparable machine.
doriannblain
commented
1 month ago Hi @spectralcode, thanks for your comments!
As I understand it, the main scholarly work is spectral_model.py and the corresponding documentation. Is that correct?
Yes indeed. You can check my comment on the pre-review talking more precisely about our aims with this submission.
Is the method or code that was used to generate the results of Table 1 in the paper available?
Absolutely, those are the _performance_test_* scripts that you can find in the tests directory. You can execute them from the pRT source directory with e.g.:
import tests._performance_test_correlated_k as test_ck
test_ck.run()This will creates a prof_*.out file in the tests/results directory, and displays in the console peak memory usage as well as the top 3 lines that use the most memory according to the tracemalloc standard library (see the debug module for more insight). The .out file can be visualized with e.g. snakeviz.
Note that the performance tests on the gitlab are for pRT 3. They can be adapted to pRT 2 by changing the argument names to their older equivalent. I can provide those scripts on request, but the old pRT 2 opacities are required as well (see the old documentation for correlated-k opacities and the old Keeper for line-by-line opacities). Alternatively, I attached the .out files I used for Table 1.
In the documentation, you mention that there is currently no explanation for the model parameters of the SpectralModel yet.
We will add a section about the model parameters, and let you know once it's done.
spectralcode
commented
1 month ago @doriannblain, thanks for the additional information. It looks like you are in the middle of renaming a specific variable. To perform one of the tests, I had to change 'irradiation_geometry' to 'emission_geometry' in 'performance_test_correlated_k_emission'. Besides this, everything worked. I got comparable results regarding peak RAM usage and some deviations regarding the measured times:
| test | time (s) | RAM (MiB) |
|---|---|---|
| Emission 'c-k' | 5.164 | 1878.631 |
| Emission 'lbl' | 11.19 | 2590.376 |
| Transmission 'c-k' | 0.6292 | 756.819 |
| Transmission 'lbl' | 7.685 | 2394.291 |
The times in the 'line by line' test case are roughly twice as large when compared to your table. Of course, the reason could be that I used different hardware than the one in your test. For me the results look plausible.
My configuration: Ubuntu 22.04.4, Intel® Core™ i7-10750H CPU @ 2.60GHz × 6
doriannblain
commented
1 month ago @spectralcode, the renaming of emission_geometry should not causes any error. I have updated the JOSS_SpectralModel branch to the latest dev version (3.1.0a28). If there was any issue, it should be fixed now.
As for the "lbl" tests, I just re-did them with the latest version and the setup reported in the article, and obtained the following:
| test | time (s) | RAM (MiB) |
|---|---|---|
| Emission 'lbl' | 4.709 | 2642.846 |
| Transmission 'lbl' | 3.452 | 2229.554 |
So the results have not changed much. The CPU frequency difference is not that large, especially when looking at the turbo frequencies. I see 2 larger differences: your L3 cache is 12 MB while mine is 64 MB. I have also RAM at 3200 MHz, maybe yours is slower? In any case, you can send me your .out files and I can try to spot any suspicious behaviour.
doriannblain
commented
1 month ago @spectralcode, about the model_parameters documentation, we added a list and short description of all of them in the reference API, in the docs (not published yet). It is referenced in the SpectralModel tutorial notebook. You can find a preview in this archived HTML file.
spectralcode
commented
1 month ago @doriannblain without renaming emission_geometryI get this output:
Successfully loaded all opacities
Emission, run 1/7
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/vmuser/petitRADTRANS/tests/_performance_test_correlated_k.py", line 151, in run
main(n_runs, trace_memory)
File "/home/vmuser/petitRADTRANS/tests/_performance_test_correlated_k.py", line 103, in main
performance_test_correlated_k_emission(
File "/home/vmuser/petitRADTRANS/tests/_performance_test_correlated_k.py", line 54, in performance_test_correlated_k_emission
radtrans.calculate_flux(
TypeError: Radtrans.calculate_flux() got an unexpected keyword argument 'irradiation_geometry'Maybe one important piece of information that I should have included from the beginning: I am running Ubuntu as a guest OS in a virtual machine. This probably affects CPU and memory performance. For me the test results seem plausible. However, if you are interested in taking a look at my output files, I have uploaded them here for you 20240730_prof_out.zip
Thank you for adding the model_parameter documentation.
Regarding the paper:
$V_\textrm{rest}$ instead of $V\mathrm{rest}$. This suggestion is of course a matter of personal preference, please use the symbol you think is the most suitable for your work. The word count of your paper is 2121. However, JOSS recommends a paper length of 250-1000 words. Do you see any possibility to shorten it? Some suggestions:
doriannblain
commented
1 month ago @spectralcode
About the variable name issue, I think you got your errors because you have the main version (3.0.7) installed, as indicated by from your .out file names. Please be sure to use the alpha of version 3.1.0 from the JOSS branch.
This is how using emission_geometry should looks:
Successfully loaded all opacities
Emission, run 1/7
/mnt/c/Users/doria/OneDrive/Documents/programs/Python/petitRADTRANS/petitRADTRANS/radtrans.py:2911: FutureWarning: 'emission_geometry' is deprecated and will be removed in a future update. Use 'irradiation_geometry' instead
warnings.warn(
Loading PHOENIX star table in file '/home/dblain/petitRADTRANS/input_data/stellar_spectra/phoenix/phoenix.startable.petitRADTRANS.h5'... Done.
Emission, run 2/7As for the .out files themselves, I do not see anything suspicious. The different functions relative execution times are similar in my case, so this should just be a matter of hardware or setup performance. Note that I got these results running Debian on a Windows machine through WSL. Maybe you are indeed using a less performant way of hosting your linux OS.
Thanks for your comments on the paper. We will fix the typos and add the comparison with other softwares (EDIT: at a first glance it seems that only ATMOSPHERIX may have a similar, less developed feature implemented).
We were aware of this recommendation but I have seen some JOSS articles with similar word counts. Nevertheless, we could maybe condense some sections as you suggested. About the petitRADTRANS 3 update, maybe we should add more explicitly that the full implementation of SpectralModel was in this update, this way this section may seem a bit less out-of-place.
doriannblain
commented
3 weeks ago @editorialbot generate pdf
editorialbot
commented
3 weeks ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
doriannblain
commented
3 weeks ago @spectralcode, we have added a quick comparison with other works and very slightly reduced the number of words. I hope that will be sufficient.
spectralcode
commented
3 weeks ago About the variable name issue, I think you got your errors because you have the main version (3.0.7) installed,
Yes, you are right. This was the issue.
Also, thank you for making the changes. Overall, great work! I recommend your submission for publication.
mbobra
commented
3 weeks ago Thank you so very much for your thorough review, @spectralcode! Your volunteer time keeps JOSS running and the JOSS team appreciates it very much ☀️
mbobra
commented
3 weeks ago @doriannblain Just a quick programming note about timing -- Ben Hord is tied up for the next month or so, and will have his review complete by October 1.
Submitting author: !--author-handle-->@doriannblain<!--end-author-handle-- (Doriann Blain) Repository: https://gitlab.com/mauricemolli/petitRADTRANS Branch with paper.md (empty if default branch): JOSS_SpectralModel Version: v3.1.0 Editor: !--editor-->@mbobra<!--end-editor-- Reviewers: @spectralcode, @benhord Archive: Pending
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
@spectralcode & @benhord, 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 @mbobra 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 @spectralcode
📝 Checklist for @benhord