search

openjournals / joss-reviews

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

[REVIEW]: ELK: A python package for correcting, analyzing, and diagnosing TESS integrated light curves #5605

Closed editorialbot closed 1 year ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@tobin-wainer<!--end-author-handle-- (Tobin Wainer) Repository: https://github.com/tobin-wainer/elk Branch with paper.md (empty if default branch): joss Version: v1.0 Editor: !--editor-->@warrickball<!--end-editor-- Reviewers: @ryanoelkers, @danhey Archive: 10.5281/zenodo.8393595

Status

status

Status badge code:

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

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

@hvidy & @ryanoelkers, 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 @warrickball 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 @ryanoelkers

📝 Checklist for @danhey

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 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.06 s (863.1 files/s, 141214.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Jupyter Notebook                 8              0           3361           1146
Python                          13            404            747           1115
TeX                              2             30              0            522
CSS                              2             15             23            180
Markdown                         7             48              0            156
YAML                             5             17             14            126
reStructuredText                11             66             76             76
DOS Batch                        1              8              1             26
make                             1              5              7             12
-------------------------------------------------------------------------------
SUM:                            50            593           4229           3359
-------------------------------------------------------------------------------

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

Wordcount for paper.md is 1188

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

OK DOIs

- 10.1086/133378 is OK
- 10.1086/323719 is OK
- 10.3847/1538-3881/ac09f1 is OK
- 10.1038/nature15731 is OK
- 10.1086/143167 is OK
- 10.3847/1538-3881/aad050 is OK
- 10.3847/1538-3881/aad68e is OK
- 10.1088/0004-637X/766/2/101 is OK
- 10.3847/1538-4365/aaebfd is OK
- 10.1117/1.JATIS.1.1.014003 is OK
- 10.3847/1538-3881/ac625a is OK
- 10.3847/2515-5172/abca2e is OK
- 10.1051/aas:2000332 is OK

MISSING DOIs

- 10.1214/aoms/1177731677 may be a valid DOI for title: Distribution of the Ratio of the Mean Square Successive Difference to the Variance
- 10.3847/1538-3881/acb20c may be a valid DOI for title: Localizing Sources of Variability in Crowded TESS Photometry

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:

warrickball commented 1 year ago

Hi @hvidy & @ryanoelkers, and thanks again for agreeing to review. This is the review thread for the paper. All of our correspondence will now happen here.

Please read the "Reviewer instructions & questions" in the first comment above, and generate your checklists by commenting @editorialbot generate my checklist on this issue. 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. We aim to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. We also encourage reviewers to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#4992 so that the issue/PR is linked to this thread. Please also feel free to comment and ask questions on this thread. JOSS editors have found it 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 start whenever you can. JOSS reviews are iterative and the authors can start responding while you continue to review other parts of the submission.

Finally, don't hesitate to ask many any questions you might have about the process.

ryanoelkers commented 1 year ago

Review checklist for @ryanoelkers

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

ryanoelkers commented 1 year ago

@tobin-wainer very nice work! thanks for putting together this package. here are a few comments on the open source paper.

  1. It may be useful to define what "integrated" means in the context of light curves as opposed to a single source. Integrated has many definitions in astronomy.
  2. line 56 and line 66 - TESS has observed hundreds of millions of stars, not just 200K. The FFIs are still TESS observations and were part of the primary mission. The 200K mentioned were the short-cadence targets which had their light curves reduced by NASA. You may want to be clear in that distinction. All of the short cadence targets are blended as well. The majority are bright so the contamination from other sources is low, but that doesn't mean there aren't highly blended sources in those 200K. The TIC did not prioritize stars for short cadence based on blending. The TIC prioritization was a combination of brightness, contamination, planet-to-star radii, and "special" considerations.
  3. line 85 - "Then gives users..." I think this is a typo.
  4. There are a few references which appear to have issues compiling in the GitHub version.
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:

tobin-wainer commented 1 year ago

Hi @ryanoelkers,

Thanks so much for your comments. I have addressed each of your comments by doing the following:

  1. This is very true. We have have quite the time trying to decide how to define this. I have added a paragraph defining what integrated means in our study.

  2. I have re-worked these paragraphs into one which clarifies this point.

  3. This was a typo and it has been fixed.

  4. When we trimmed down the .bib file it seems we accidentally removed some references that we actually needed. These have been fixed.

tobin-wainer 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:

ryanoelkers commented 1 year ago

thanks for addressing those points @tobin-wainer! the new draft of the open source paper looks good.

ryanoelkers commented 1 year ago

@tobin-wainer i've looked at some of the documentation and tutorials (mainly the calculating lightcurve statistics tutorial). it looks nice so far! just two small issues i found (which may not be issues, but worth checking).

  1. in the line "%config InlineBackend.figure_format = "retina"" throws an invalid syntax line for my PyCharm IDE.
  2. the lc.stats["n_peaks"] line reports 2 for me, but is 3 in the tutorial. The other numbers match within floating point errors (so it is likely not a problem, just something you should be aware of)
tobin-wainer commented 1 year ago

Hi @ryanoelkers, thanks for your comments.

  1. This command is for formatting the figures in the jupyter notebook. We have added a comment to all the tutorial notebooks saying this command is not required, and only applicable to jupyter notebooks. We would prefer to keep it, as the figures look better on the docs website, but we can remove if necessary.

  2. After reading this I re-ran the tutorial myself and also got 2 peaks. I realized that the tutorials were actually run on a previous version of elk, and with the latest version release this number changed. The tutorials are now all run with the latest version.

While going through these comments we also added an upper limit on the numpy version since the latest release has breaking changes with the current lightkurve version.

warrickball commented 1 year ago

@editorialbot add @danhey as reviewer

editorialbot commented 1 year ago

@danhey added to the reviewers list!

warrickball commented 1 year ago

Hi @danhey, thanks again for agreeing to review.

As a reminder of the process, please read the "Reviewer instructions & questions" in the first comment above, and generate your checklists by commenting @editorialbot generate my checklist on this issue. 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.

If you submit issues or pull requests on the software repository, please mention openjournals/joss-reviews#4992 so that the issue/PR is linked to this thread. Please also feel free to comment and ask questions on this thread. JOSS editors have found it better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We usually aim for the review process to be completed within about 4-6 weeks but I know this one has been delayed by various conferences and holidays over the Northern Hemisphere's summer. JOSS reviews are iterative and the authors can start responding while you continue to review other parts of the submission.

Finally, don't hesitate to ask many any questions you might have about the process.

danhey commented 1 year ago

Review checklist for @danhey

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

tobin-wainer commented 1 year ago

I have updated the JOSS paper with the citation of the AAS paper, which was recently published.

@editorialbot generate pdf

tobin-wainer 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:

ryanoelkers commented 1 year ago

Hi @ryanoelkers, thanks for your comments.

  1. This command is for formatting the figures in the jupyter notebook. We have added a comment to all the tutorial notebooks saying this command is not required, and only applicable to jupyter notebooks. We would prefer to keep it, as the figures look better on the docs website, but we can remove if necessary.
  2. After reading this I re-ran the tutorial myself and also got 2 peaks. I realized that the tutorials were actually run on a previous version of elk, and with the latest version release this number changed. The tutorials are now all run with the latest version.

While going through these comments we also added an upper limit on the numpy version since the latest release has breaking changes with the current lightkurve version.

Thanks @tobin-wainer ! Things looks good now.

ryanoelkers commented 1 year ago

@tobin-wainer thanks for your hard work so far! A few other general comments I noticed.

  1. Are you planning to review (and possibly accept) pull requests? If so, you may want to state that explicitly in the documentation.
  2. In a similar vein, I couldn't find where folks could go to alert you of issues (either with bugs or when having trouble during installation). I realize the easiest method may just be to open an issue on GitHub, but it never hurts to say that explicitly in the documentation. Not all folks will know that "issues" exist on GitHub with being told about them :).
  3. It looks like you still have a "TO DO" active in the documentation for citations. I realize some of that will occur once the JSOSS paper is published, but you may want to clean that up or add text without the various hyperlinks to references.
tobin-wainer commented 1 year ago

Hi @ryanoelkers, thanks for the comments.

  1. and 2. Thanks for pointing this out. We are planning on accepting pull requests and issues. We have created a page on the docs with instructions, and links on how to do this via github. We have also made templates for specific types of issues.

  2. Yes, thank you for mentioning this. The AJ paper was recently accepted, and we have added the citation for this paper. We will add the JOSS citation when it is appropriate to do so.

danhey commented 1 year ago
Intro Comments

Hi all, thanks for producing this nice bit of software, I can see it becoming widely used for light curve extraction. The code is well documented with a few nice examples. I've split up my comments below into the relevant sections.

Installation:

Installation was a little difficult for me, and took about an hour of troubleshooting to sort out (not the authors fault!). You can see above that I opened a PR regarding the problem, but prior to that the Python environment (3.8) I created was not supported. I think it would be useful to mention supported Python versions and a workaround for the above problem in the docs.

Getting started & Tutorials:

All work fine with no issue. It would be nice to have some of the text explain what's happening when the quality test fails. e.g., the first example fails 2 quality tests, how come? Should we be worried? Can we force the light curve to extract anyway?

The getting started tutorial has a typo in the link at the bottom of the page. There are quite a few typos in the docs in general (e.g., https://elk.readthedocs.io/en/latest/tutorials/diagnostic_gif.html has aperature, peack, identttify, etc).

The image for the diagnostic_gif is missing.

General paper comments:

The paper is nicely written and clearly explains the niche of integrated light curves. However, I think the paper is missing a short discussion of other light curve extraction software (e.g., eleanor). Indeed, looking at the source code shows that this method relies strongly on lightkurves RegressionCorrector -- this should be mentioned more clearly in the text! Right now the text strongly implies that the elk is doing "singular value decomposition methods and principle component analysis" from scratch, but that is not the case.

Finally, it's not unusual for JOSS papers to be illustrated with a figure. I know there's a lot of nice figures in the AAS version of this submission, but it might be worthwhile having a simple plot of an extracted light curve. This is totally up to the authors.

Great work all, I'm looking forward to seeing more results come from this tool!

ryanoelkers commented 1 year ago

Hi @ryanoelkers, thanks for the comments.

  1. and 2. Thanks for pointing this out. We are planning on accepting pull requests and issues. We have created a page on the docs with instructions, and links on how to do this via github. We have also made templates for specific types of issues.
  2. Yes, thank you for mentioning this. The AJ paper was recently accepted, and we have added the citation for this paper. We will add the JOSS citation when it is appropriate to do so.

Hi @tobin-wainer. thanks for this! i think i have more or less finished my review. apart from the comments from @danhey i don't see additional changes. very nice work!

warrickball commented 1 year ago

@editorialbot remove @hvidy from reviewers

editorialbot commented 1 year ago

@hvidy removed from the reviewers list!

tobin-wainer commented 1 year ago

Hi @danhey!

Thank you for these comments!

Intro Comments

Hi all, thanks for producing this nice bit of software, I can see it becoming widely used for light curve extraction. The code is well documented with a few nice examples. I've split up my comments below into the relevant sections.

Installation:

Installation was a little difficult for me, and took about an hour of troubleshooting to sort out (not the authors fault!). You can see above that I opened a PR regarding the problem, but prior to that the Python environment (3.8) I created was not supported. I think it would be useful to mention supported Python versions and a workaround for the above problem in the docs.

We believe that we have solved this issue. The source of the issue was recently updated in the newest versions of Pytables, but we have implemented the changes from your pull request and download directly from the source, and it is working. Hopefully this is the case on the M1 chips. We also note in the installation page that the elk is supported on Python 3.9+ environments.

Getting started & Tutorials:

All work fine with no issue. It would be nice to have some of the text explain what's happening when the quality test fails. e.g., the first example fails 2 quality tests, how come? Should we be worried? Can we force the light curve to extract anyway?

Thanks for this comment. We have updated the documentation to describe these tests, and what is going on. We have also detailed the forced extraction for surpassing the scattered light test. Finally on the docs we point to the AAS version of the text for an in depth description of the tests.

The getting started tutorial has a typo in the link at the bottom of the page. There are quite a few typos in the docs in general (e.g., https://elk.readthedocs.io/en/latest/tutorials/diagnostic_gif.html has aperature, peack, identttify, etc).

The image for the diagnostic_gif is missing.

Thanks for pointing these out. I have now implemented a word check in my coding base (which I probably should have had a while ago), and believe that we have fixed these typos. We have also hardcoded the diagnostic gif to display on the website, and fixed all the links.

General paper comments:

The paper is nicely written and clearly explains the niche of integrated light curves. However, I think the paper is missing a short discussion of other light curve extraction software (e.g., eleanor). Indeed, looking at the source code shows that this method relies strongly on lightkurves RegressionCorrector -- this should be mentioned more clearly in the text! Right now the text strongly implies that the elk is doing "singular value decomposition methods and principle component analysis" from scratch, but that is not the case.

We have updated the paper to describe in more detail the processes from lightkurve which were used, and give more credit to the package. We have also added a citation to eleanor.

Finally, it's not unusual for JOSS papers to be illustrated with a figure. I know there's a lot of nice figures in the AAS version of this submission, but it might be worthwhile having a simple plot of an extracted light curve. This is totally up to the authors.

We agree that the AAS version has very good illustrations of the extraction process. We believe it would be inappropriate to publish the exact same figure in 2 papers, and don't believe the information conveyed in a different version would be substantial. Therefore, in the text we direct readers to the figure in the AAS version to see the extraction process.

Great work all, I'm looking forward to seeing more results come from this tool!

Thanks you for the comments, and I look forward to discussing further!

danhey 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:

danhey commented 1 year ago

Thanks @tobin-wainer! I'm happy with these changes and my review is done. Great work!

warrickball commented 1 year ago

Thanks @danhey and @ryanoelkers for your reviews! There are only editorial matters to worry about before the paper is published.

@tobin-wainer, three last things on the editorial side of the paper before we start wrapping this up.

First, there are a few typos. I did a quick spell check and found

Second, many of the citations are malformed, e.g.,

Finally, the second paragraph starts with

The term ‘integrated’ refers to the method in which we are extracting blended photometry from an aperture, a practice common in the literature.

To demonstrate it's a common practice, you should cite a few relevant examples of this practice at this point.

tobin-wainer 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:

tobin-wainer 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:

tobin-wainer commented 1 year ago

Hi @warrickball,

Thank you for these comments. I have gone through and believe I have gotten all of the typos. I have also fixed the formatting issue for all citations with "e.g.," and they are correct now. Finally I have added two citations for the sentence you refer to, one being a highly cited review article.

Thanks you for the comments, and if anything else is needed, please let me know.

warrickball commented 1 year ago

Thanks for the sweep! I've noticed there are still a few extra brackets around citations, e.g.

(TIC, Stassun et al. (2018))

(e.g., Oelkers & Stassun (2018); Nardiello (2020); Higgins & Bell (2022)).

elk is jointly published in (Wainer et al., 2023),

I'll have a look into these myself tomorrow morning to see if I can fix them via PR. I'll also brush up on the next steps so I can help wrap this up ASAP.

warrickball commented 1 year ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

warrickball 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.1086/133378 is OK
- 10.1051/0004-6361/202140546 is OK
- 10.1093/mnras/staa2745 is OK
- 10.3847/2515-5172/ac2ef0 is OK
- 10.1086/323719 is OK
- 10.3847/1538-3881/ac09f1 is OK
- 10.1038/nature15731 is OK
- 10.1086/143167 is OK
- 10.3847/1538-3881/aad050 is OK
- 10.3847/1538-3881/aad68e is OK
- 10.1088/0004-637X/766/2/101 is OK
- 10.3847/1538-4365/aaebfd is OK
- 10.1117/1.JATIS.1.1.014003 is OK
- 10.3847/1538-3881/ac625a is OK
- 10.3847/2515-5172/abca2e is OK
- 10.1051/aas:2000332 is OK
- 10.3847/1538-3881/ace960 is OK
- 10.1088/1538-3873/ab291c is OK
- 10.1093/mnras/staa716 is OK
- 10.1146/annurev-astro-091918-104430 is OK

MISSING DOIs

- 10.1214/aoms/1177731677 may be a valid DOI for title: Distribution of the Ratio of the Mean Square Successive Difference to the Variance
- 10.3847/1538-3881/acb20c may be a valid DOI for title: Localizing Sources of Variability in Crowded TESS Photometry

INVALID DOIs

- None
warrickball commented 1 year ago

Not sure why the missing DOIs are flagged. I've followed them and they look correct.

danielskatz commented 1 year ago

@warrickball - they are flagged because they're not in the bib entries as doi lines - the author should check them and add them if they are correct.

warrickball commented 1 year ago

Regarding the DOI for Higgins & Bell, this whole reference should be updated to the published version (Higgins & Bell 2023). I've opened a PR to fix the remaining brackets for citations and add the DOI for von Neumann (1941).

tobin-wainer commented 1 year ago

Hi @warrickball, Thank you for your work and the PR.

I have merged the PR, and released Version 1.0 to reflect the changes in the review process. I have created a DOI ( DOI: 10.5281/zenodo.8393595), and verified the author list and ORCIDs. I have also created a .cff file to comply with with DOI.

If there is anything else needed, please let me know.

warrickball commented 1 year ago

Hi, @tobin-wainer, thanks! Note that the review checklist is mostly for me but I appreciate that you've effectively ticked some things off for me. Note, however, that

  • Make sure that the title and author list (including ORCIDs) in the archive match those in the JOSS paper.
  • Make sure that the license listed for the archive is the same as the software license.

The author list on the Zenodo archive should match the JOSS paper but is currently only you and Tom Wagg. You should be able to amend the Zenodo entry without having to mint a new one. Note also that Zenodo hasn't detected the MIT license, which you can also change in the Zenodo metadata. (I'm guessing this is a limitation of the GitHub integration.)

I have also created a .cff file to comply with with DOI.

Thanks! The editorialbot actually now mints a CITATION.cff for the JOSS paper on acceptance, which you'll see and may wish to use.

warrickball 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.1086/133378 is OK
- 10.1051/0004-6361/202140546 is OK
- 10.1093/mnras/staa2745 is OK
- 10.3847/2515-5172/ac2ef0 is OK
- 10.1086/323719 is OK
- 10.3847/1538-3881/ac09f1 is OK
- 10.1038/nature15731 is OK
- 10.1086/143167 is OK
- 10.1214/aoms/1177731677 is OK
- 10.3847/1538-3881/aad050 is OK
- 10.3847/1538-3881/aad68e is OK
- 10.1088/0004-637X/766/2/101 is OK
- 10.3847/1538-4365/aaebfd is OK
- 10.1117/1.JATIS.1.1.014003 is OK
- 10.3847/1538-3881/ac625a is OK
- 10.3847/2515-5172/abca2e is OK
- 10.1051/aas:2000332 is OK
- 10.3847/1538-3881/ace960 is OK
- 10.1088/1538-3873/ab291c is OK
- 10.1093/mnras/staa716 is OK
- 10.1146/annurev-astro-091918-104430 is OK

MISSING DOIs

- 10.3847/1538-3881/acb20c may be a valid DOI for title: Localizing Sources of Variability in Crowded TESS Photometry

INVALID DOIs

- None