search

openjournals / joss-reviews

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

[REVIEW]: IGRINS RV: A Python Package for Precision Radial Velocities with Near-Infrared Spectra #3095

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @shihyuntang (Shih-Yun Tang) Repository: https://github.com/shihyuntang/igrins_rv Version: v1.0.0 Editor: @dfm Reviewer: @jmbrewer, @gully Archive: 10.5281/zenodo.4917291

:warning: JOSS reduced service mode :warning:

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

Status badge code:

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

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

@jmbrewer & @gully, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

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 ✨

Review checklist for @jmbrewer

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @gully

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @jmbrewer, @gully it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

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.

:star: Important :star:

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:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

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

@whedon commands

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

@whedon generate pdf
whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1088/0004-637X/761/2/164 is OK
- 10.1088/0004-6256/148/3/53 is OK
- 10.5281/zenodo.845059 is OK
- 10.1117/12.2232780 is OK
- 10.1117/12.2312345 is OK
- 10.1117/12.2056431 is OK
- 10.1117/12.856864 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=3.79 s (15.6 files/s, 4227.8 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          38           2448           2567           8998
TeX                              2            111              6            461
Cython                           1            105            164            274
reStructuredText                11            103             86            211
Markdown                         3             47              0            176
make                             1             30              6            156
Bourne Shell                     2             10              1             36
YAML                             1              0              0             19
-------------------------------------------------------------------------------
SUM:                            59           2854           2830          10331
-------------------------------------------------------------------------------

Statistical information for the repository '9d053e57192775004de92cb4' was
gathered on 2021/03/10.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
astahl7                        716         48676          42331           41.04
sytang                        1873         72453          58269           58.96

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
astahl7                    4563            9.4          3.8               12.29
sytang                     9450           13.0          6.8               13.07
whedon commented 3 years ago

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

dfm commented 3 years ago

@jmbrewer, @gully – 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.

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. 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#3095 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 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.

whedon commented 3 years ago

:wave: @gully, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @jmbrewer, please update us on how your review is going (this is an automated reminder).

gully commented 3 years ago

Update: I had trouble installing TelFit and highlighted some of the challenges and impact in https://github.com/shihyuntang/igrins_rv/issues/3. As part of this review it would be valuable to move away from packaging the TelFit source code with igrins_rv and instead pin to a version of the main TelFit repo. I think this change can be accomplished without upstream changes.

gully commented 3 years ago

Update: I got igrins_rv to run its steps 1 and 2 on the included example data. I am now running step 3, which takes a while. I have recommended the inclusion of a diagram of the workflow in the documentation. I also recommend adding a progress bar, and transitioning from print to logging https://github.com/shihyuntang/igrins_rv/issues/4.

gully commented 3 years ago

Overall the code appears to work as advertised for the example data. The documentation and testing center on the commandline interface (CLI), and are adequate to reproduce the claimed results with the included example data. I have not attempted to run the code on data beyond the examples. The algorithmic ingredients (such as those in Engine) do not appear to be documented or tested.

arfon commented 3 years ago

FYI, I just heard from the AAS publishing team that the DOI for their paper will be 10.3847/1538-3881/abf5e7.

shihyuntang commented 3 years ago

Hi, I had resolved all the great comments and suggestions from @gully and @jmbrewer posted here. What is the next step?

Thank you.

gully commented 3 years ago

The actual paper portion should include comparisons to existing RV frameworks. How does igrins_rv differ from those, and why are those tools unsuitable to the task, prompting the creation of igrins_rv? For example, one such package that comes to mind is wobble. One of wobble's limitations (if I recall correctly) is that it requires epochs acquired with a range of BERV's, and assumes somewhat isolated telluric lines, which may not adequately apply to the in-hand IGRINS data and the dense tellurics in the near-IR. What other packages like wobble are out there, and why are they deemed to be unsuitable for the task? Other packages like radvel and exoplanet consume post-processed radial velocity velocities for science, but are not designed to do any of the custom pre-processing shown here; making that distinction may further clarify for readers the niche that igrins_rv satisfies.

At the moment, a prospective contributor to this codebase may face a challenge to extend or modify the code owing to 1) inherent complexity of precision echelle modeling, 2) limited standalone documentation for sub-routines, and 3) complexity arising from the interconnections among sub-routines (i.e. software architecture). Item 1 can't be changed, it's inherent to the problem! Regarding item 2, the igrins_rv team has made some progress on standalone documentation by adding docstrings to some sub-functions.

Finally item 3 appears to be the hardest to describe and ameliorate: there is a delicate balance between solving the problem at hand and designing the code to be extensible to other related problems. In essence my question is: to what extent could igrins_rv be further modularized? igrins_rv laudably takes advantage of existing specialized codes such as telfit and the IGRINS-specific plp. One step that could further simplify the code is to modularize the IGRINS-specific components. For example, what portions of the code are IGRINS-specific, and what portions can apply to other near-IR echelle spectrographs? Asking that question throughout the codebase should reveal opportunities to make the code more modular. Modularity makes it easier to maintain and sustain the codebase, and ultimately attracts more contributors and users.
I have attempted to modularize some of the IGRINS file Input/Output and other routine operations in an experimental API based on the IGRINS PLP output, just like igrins_rv. It is currently unstable and not suitable as a dependency. It's a proof-of-concept. In the longer term, one could imagine separating out the hum-drum parts of the code with a significant code reduction:

clean_spectrum = IGRINSSpectrum(filename).remove_nans().trim_edges().remove_outliers()

A small point regarding the inclusion of a custom detect_peaks algorithm. Why was this custom code chosen over the seemingly equivalent scipy find_peaks?

shihyuntang commented 3 years ago

Dear @gully ,

Here are our reply:

--> The detect_peaks function under detect_peaks.py is adopted in the very early stage of the code development when we were not aware of the similar function in scipy. It works fine so we do not change it in the end.

Thank you.

gully commented 3 years ago

I will double-check tomorrow any remaining tasks in this review. I have not looked at the parallel paper linked in a previous comment thread. Maybe a question to the editors---how should parallel papers be handled?

dfm commented 3 years ago

@gully: Thanks for the question - it is fine for the documentation to reference the parallel paper for details of implementation, but you are not expected to also review that paper so if you don't feel that the documentation for the package gives sufficient detailed description of the use cases, applications, or general structure of the code then that needs to be added to the docs!

Does that sufficiently address this here? Happy to provide more specific feedback if required.

gully commented 3 years ago

@whedon generate pdf

gully commented 3 years ago

@whedon remind @gully in 2 days

whedon commented 3 years ago

I'm sorry @gully, I'm afraid I can't do that. That's something only editors are allowed to do.

whedon commented 3 years ago

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

gully commented 3 years ago

Looking over the PDF, the text appears to lack some references for how this package compares to other RV packages. A comment above shows that a reference to wobble was added, but I cannot see that in the newly generated pdf. Wobble is not the only relevant package though, and I am recommending that other comparable packages are cited to clarify how igrins_rv fits into the broader ecosystem of RV packages. If there is truly such a dearth of open source packages, that can be explained with citations to papers employing closed-source packages. I am also recommending the dependencies such as numpy and others be included unless the editors provide guidance to the contrary.

shihyuntang commented 3 years ago

Dear @gully ,

Sorry for the confusing. The section about wobble was buried in the second half of the first paragraph in the statement of need. We now make it as a standalone paragraph and give a broad view on other NIR RV pipelines (including the PySHELL , wobble, and SERVAL).

Current RV pipelines and techniques that can deliver RV precision in tens of m/s (or beyond) in the NIR, e.g., PySHELL [@cale19], wobble [@bede19], SERVAL [@zech18] or the PCA-based cross-correlating method used for the SPIRou spectrograph [@mout20], all require instruments that are highly stabilized and with well-characterized wavelength solutions. For example, the iSHELL spectrograph can be equipped with the methane isotopologue gas cell, and the SPIRou and the CARMENES (NIR channel) spectrograph come with uranium-neon hollow-cathode lamps and stabilized Fabry-Perot etalons. The Immersion GRating INfrared Spectrometer (IGRINS) spectrograph [@yuk10; @park14; @mace16; @mace18], on the other hand, was not designed to be RV-stable and comes with no means of wavelength calibration accurate enough to achieve RVs precise to tens of m/s using existing techniques. A new approach to extract precision RVs is needed for an echelle spectrograph like IGRINS, which still offers fertile ground for RV science with its high resolution (R ~ 45,000) and broad spectral grasp (the full H and K bands).

IGRINS RV is a pipeline tailored for extracting precision RVs from spectra taken with IGRINS on different facilities. This pipeline is built on the forward-modeling methodology that was successfully applied to CSHELL and PHOENIX spectra [@croc12] that utilized telluric lines as a common-path wavelength calibrator. Compared to RVs obtained by cross-correlation with stellar templates adopted by past studies, IGRINS RV gives three times higher RV precision, about 25--30 m/s, around narrow-line stars in both H and K bands, shown by years of monitoring on two RV standard stars, GJ\ 281 and HD\ 26257. IGRINS RV also pushes this technique, using telluric lines as wavelength calibrator for RV calculation, to its limits as studies found the stability of the telluric lines is about 10--20 m/s. Moreover, IGRINS RV is also tailored to take into account specific aspects of the IGRINS instrument, like the variations in spectral resolution across the detector and the year-long K band detector defocus.

Sorry for not making this clear in the first place. I hope this time we make the need of IGRINS RV clear.

As for the dependencies on other packages used in IGRINS RV, we did have them listed in the final paragraph of the statement of need section. @dfm I hope we put the dependencies statement in the right place. I saw other JOSS papers also have them in the end of the statement of need section.

IGRINS RV makes use of the astropy [@astropy2013; @astropy2018] on handing sky coordinates and barycentric velocity correction, scipy [@2020SciPy] and numpy [@2020NumPy] on mathmatical calculation, nlopt [@john08; @box1965new] on the optimization process, pandas [@reback2020pandas; @mckinney2010] on data management, and matplotlib [@Hunter2007] on plotting. We also used a part of code from BMC [@marcos2021] for peak detection. IGRINS RV requires that the igrins plp v2.2.0 [@leegul17] and Telfit [@gull14] packages be pre-installed. Detailed documentation and tutorials can be found on the GitHub wiki page.

Thank you.

gully commented 3 years ago

Ok, thank you @shihyuntang that satisfies all of my questions, and I believe satisfies all the remaining checkboxes of the review. I believe this concludes my portion of the review of igrins_rv. Thank you for patience with the process, for your contribution to the open source community, and for making the edits I requested. Best wishes to you and the team on continued success with the project.

gully commented 3 years ago

@whedon accept

whedon commented 3 years ago

I'm sorry @gully, I'm afraid I can't do that. That's something only editors are allowed to do.

jmbrewer commented 3 years ago

Sorry for the long delay; the end of the semester really hit hard this year. The comments from @gully made my job easy in finishing things up. I did not have any additional questions. I have now completed the remaining checkboxes in the review as well. Great work @shihyuntang and your team!

dfm commented 3 years ago

Thanks @jmbrewer!

@shihyuntang: give me a couple of days to do some final edits/processing and then I'll have some last steps for you to do before acceptance.

dfm commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

dfm commented 3 years ago

@whedon check references

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

OK DOIs

- 10.3847/1538-3881/ab40a7 is OK
- 10.1088/0004-637X/761/2/164 is OK
- 10.1088/0004-6256/148/3/53 is OK
- 10.5281/zenodo.845059 is OK
- 10.1117/12.2232780 is OK
- 10.1117/12.2312345 is OK
- 10.1117/12.2056431 is OK
- 10.1117/12.856864 is OK
- 10.1093/comjnl/8.1.42 is OK
- 10.1051/0004-6361/201322068 is OK
- 10.3847/1538-3881/aabc4f is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.5281/zenodo.3509134 is OK
-  10.25080/Majora-92bf1922-00a  is OK
- 10.1109/MCSE.2007.55 is OK
- 10.5281/zenodo.4599319 is OK
- 10.1088/1538-3873/128/968/104501 is OK
- 10.3847/1538-3881/ab3b0f is OK
- 10.1093/pasj/psaa085 is OK
- 10.1051/0004-6361/202038108 is OK
- 10.1051/0004-6361/201731483 is OK
- 10.1051/0004-6361:200810174 is OK
- 10.1051/0004-6361/201014005 is OK

MISSING DOIs

- None

INVALID DOIs

- None
dfm commented 3 years ago

@shihyuntang: I've opened a pull request with some edits to the paper. Can you take a look at that and once you've merged, please take the following steps:

Let me know if you have questions or run into any issues!

shihyuntang commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

shihyuntang commented 3 years ago

Thank you @jmbrewer !

Two questions for @dfm :

  1. Is the pdf I just asked whedon to generate serves as the proof as other traditional journals do?
  2. About the Telfit package installation issue (a pkg used in IGRINS RV). I am still waiting for its author (kgullikson88) to merge my pull request here and make a new release of pip. If this is done, we will no longer need to manually source install (+ several changes in the source files) Telfit but just add pip install Telfit in the .yml file. However, I haven't heard from kgullikson88 for a while. Do you think I should wait so that the JOSS publish version has this fix? Or can I publish the current version of IGRINS RV on JOSS and make a new release on Github after Telfit is pip insatiable? @gully can you help on this since you reviewed my pull request on Telfit.

Thank you!

dfm commented 3 years ago

Is the pdf I just asked whedon to generate serves as the proof as other traditional journals do?

Yes! There will possibly be one final round of edits later in the process, but now is the time to check for any final issues.

gully commented 3 years ago

OK @shihyuntang telfit has pip install working for python 3, including the change you made for the setup.py configuration. Thank you! You have one open PR over there ("resolution2") that that I think I can merge next.

shihyuntang commented 3 years ago

Hi @gully Wow, thank you! My other pull request ("resolution2") for Telfit is not only changing the resolution --> resolution2, but also fixed the print statements for example Fit.py file to let it be py3.X compatible. (I have fixed the conflict for that branch. Now you should be able to just pull it.)

shihyuntang commented 3 years ago

Dear @dfm ,

Comment @whedon generate pdf on this thread and read through the manuscript to make sure that you're happy with it (it's hard to make changes later!), especially author names and affiliations.

-->Done

Increment the version number of the software and report that version number back here.

--> v1.0.0

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.

--> DOI: 10.5281/zenodo.4917291. On https://zenodo.org/record/4917291#.YMDuNC_fC-Y

Thank you!

dfm commented 3 years ago

@whedon set 10.5281/zenodo.4917291 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.4917291 is the archive.

dfm commented 3 years ago

@whedon set v1.0.0 as version

whedon commented 3 years ago

OK. v1.0.0 is the version.

dfm commented 3 years ago

@whedon recommend-accept

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

OK DOIs

- 10.3847/1538-3881/ab40a7 is OK
- 10.1088/0004-637X/761/2/164 is OK
- 10.1088/0004-6256/148/3/53 is OK
- 10.5281/zenodo.845059 is OK
- 10.1117/12.2232780 is OK
- 10.1117/12.2312345 is OK
- 10.1117/12.2056431 is OK
- 10.1117/12.856864 is OK
- 10.1093/comjnl/8.1.42 is OK
- 10.1051/0004-6361/201322068 is OK
- 10.3847/1538-3881/aabc4f is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.5281/zenodo.3509134 is OK
-  10.25080/Majora-92bf1922-00a  is OK
- 10.1109/MCSE.2007.55 is OK
- 10.5281/zenodo.4599319 is OK
- 10.1088/1538-3873/128/968/104501 is OK
- 10.3847/1538-3881/ab3b0f is OK
- 10.1093/pasj/psaa085 is OK
- 10.1051/0004-6361/202038108 is OK
- 10.1051/0004-6361/201731483 is OK
- 10.1051/0004-6361:200810174 is OK
- 10.1051/0004-6361/201014005 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago

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

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/2382

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/2382, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
dfm commented 3 years ago

This has now been handed of to the Editors-in-Chief to take a final look (they might have some edits) and then do the final processing.

@jmbrewer and @gully: A huge thanks for your constructive reviews. I really appreciate the time you took to provide such great feedback!!

@shihyuntang: Thanks for submitting and all your work throughout the review process. Congrats on the excellent software!

kthyng commented 3 years ago

@dfm made this easy on me!! Looks ready to go!

kthyng commented 3 years ago

@whedon accept deposit=true