search

openjournals / joss-reviews

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

[REVIEW]: AltaiPony - Flare science in Kepler, K2 and TESS light curves #2845

Closed whedon closed 3 years ago

whedon commented 4 years ago

Submitting author: @ekaterinailin (Ekaterina Ilin) Repository: https://github.com/ekaterinailin/AltaiPony Version: v2.0.1 Editor: @dfm Reviewer: @afeinstein20, @mrtommyb Archive: 10.5281/zenodo.4095339

: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/344b45dac8496725be33ccf1cd902dc2"><img src="https://joss.theoj.org/papers/344b45dac8496725be33ccf1cd902dc2/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/344b45dac8496725be33ccf1cd902dc2/status.svg)](https://joss.theoj.org/papers/344b45dac8496725be33ccf1cd902dc2)

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

@afeinstein20 & @mrtommyb, 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 @afeinstein20

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @mrtommyb

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @afeinstein20, @mrtommyb 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 4 years ago

PDF failed to compile for issue #2845 with the following error:

Can't find any papers to compile :-(

dfm commented 4 years ago

@whedon generate pdf from branch josspaper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch josspaper. Reticulating splines etc...
whedon commented 4 years ago

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

mrtommyb commented 4 years ago

Hi @ekaterinailin I'm starting my review and working my way down the checklist. I'm at the item on Contribution and authorship. I have a question here. I notice that the software Appaloosa is described as this software’s predecessor and it looks like some of the code in this package is lifted from Appaloosa. I was wondering if you could explain a little about your authorship choices for this paper given the heritage of some of the code included.

ekaterinailin commented 4 years ago

Hi @mrtommyb ! First of all, thank you for volunteering as a reviewer.

My reasoning on authorship was: I made what I thought was a default choice based on the Github history of the project. From Appaloosa, I ended up using and adapting three functions (the aflare model, and the flare search functions in the residual light curve). AltaiPony was written from scratch, and did not grow out of Appaloosa directly, so maybe predecessor is not the right term to use in a technical sense, uncertain here. In the end, I decided to cite Jim's code in the paper, as I did with K2SC and lightkurve, from which I integrated functions and classes in a similar manner.

Is there a consideration that I am missing here?

whedon commented 4 years ago

:wave: @afeinstein20, please update us on how your review is going.

whedon commented 4 years ago

:wave: @mrtommyb, please update us on how your review is going.

afeinstein20 commented 3 years ago

Hi @ekaterinailin ! Thanks for writing AltaiPony and what a fantastic logo! The software was easy to install and run, with few to minor errors. I've recorded some of those below as well as some changes I think would make the functionality and the documentation better/easier for users!

Installation: I had no trouble installing and using the software following both installation methods. However, I did receive the following errors that I wanted to note. And there are some dependencies I found were missing later one.

  1. Through PyPI, I receive the following error: FileNotFoundError: [Errno 2] No such file or directory: '//anaconda3/lib/python3.7/site-packages/K2SC-1.0-py3.7.egg'
  2. Through Github clone, I receive the following error: error: scipy 1.4.1 is installed but scipy!=1.4.0,!=1.4.1,>=0.19.0 is required by {'lightkurve'}
  3. I ran into trouble with the progressbar on my personal machine! I assume this is a version error because it runs okay in a Google Colab notebook. Error: TypeError: __init__() got an unexpected keyword argument 'max_value'
  4. I went to run the FFD functionality and was missing the emcee and corner dependencies.

Functionality

  1. It's unclear to me what the flare detection criteria are and if there's any way for me to change them. Are there any criteria I really shouldn't change? It looks like those criteria are stored in find_flares_in_cont_obs_period but there's no documentation for this function.
  2. The injection-recovery tools and associated plots are super useful! It would be great to have a way to save these output figures, maybe through a keyword or by returning the figure instead of just plotting.
  3. When I ran simple_ffd.ed_and_freq() from the documentation, it returned only 5 data points? I copied most of the lines from the demo notebooks in your documentation, so this is kind of weird!

Documentation

  1. Could use a bit more description in the opening about paragraph, since there's way more to this software than just "finding flares" (e.g. light curve downloading, detrending, flare fitting, injection-recovery tests, etc.)
  2. Example usage could use some figures! As I was walking through the tutorials, it would have been nice to compare what my light curve looked like compared to what the docs expected.

Software Paper The paper is overall clear and well written. It provides a good outline as to why the software is necessary and what it accomplishes compared to other flare detection methods. I believe the paper would benefit from a bit more description about details of flare detection and fitting: What is detected as a flare and not a flare? What are the default settings for flare detection? How the software handle the ramp ups at the end of TESS orbits? What is defined as a the start and stop time of a flare? Adding little details like this would be useful!

ekaterinailin commented 3 years ago

Hi @afeinstein20 ! Thanks a ton for the detailed review, these are all very helpful comments, and I am looking forward to addressing them to improve the software. I have a few busy weeks ahead of me, but I hope to get my hands on the code before the end of this year.

ekaterinailin commented 3 years ago

Hello again,

I finally got my hands on the code again, so here are my (proto-)solutions to @afeinstein20 's review:

Let me know if I understood your remarks correctly! Happy new year :smiley_cat:

dfm commented 3 years ago

I'm pinging @mrtommyb to keep this on his radar as we roll into the new year.

dfm commented 3 years ago

:wave: @afeinstein20 and @mrtommyb: I wanted to check in with you both to see how your reviews are going. Let me know if you have any questions or issues. Thanks for your work on this so far!!

mrtommyb commented 3 years ago

Thanks for the poke @dfm.

afeinstein20 commented 3 years ago

Hi @ekaterinailin ! Thanks for addressing all of the points! I'm very happy with the updates 😄 The additional docs and FAQ section are super useful. I just have 2 additional comments/issues:

* You mean here? ... example.

I've gone ahead and checked all of the boxes on my review list, since you've covered everything on my end! Hopefully these 2 minor things listed above are easily fixed!

mrtommyb commented 3 years ago

Sorry that it took me a while to get back to this. I reinstalled the package, and I installed from the github repo. A few things to note

ekaterinailin commented 3 years ago

You're right, @mrtommyb , the new version of lightkurve is incompatible with AltaiPony, and it slipped me. I did two things to fix this:

  1. restricted allowed lightkurve versions (2.x seems incompatible with K2SC light curves, but I'd like to keep that functionality because people still work on those data. The new version also breaks a lot more than just from_mast, so I think this will not be fixed before major refactoring happens in AltaiPony 2.0.)
  2. set up a weekly CI build to notice such failures promptly.
ekaterinailin commented 3 years ago

As for the other requests:

ekaterinailin commented 3 years ago

@afeinstein20 Thanks for the comments!

Unfortunately, now I'm running into some issues the way the values are being binned. I've been running everything in this Google Colab notebook. I think if you open it, the error messages should appear as well?

Yes, I can see the error message, and I could indeed reproduce your heatmap error! The pansdas version 1.1.5 is the culprit and I actually fixed that in the requirements back in December (see the log), but it was not included in the 1.0.0. version. It should now work with a fresh installation of version 1.0.1 :)

@mrtommyb This should also fix the pip installation problems with lightkurve for the moment.

Yup this was totally the issue! I thought in your docs you were computing the FFD for TIC 29780677. But I know realize it was for GJ 1243, although I'm still getting fewer points still. This is probably because I've set iterations=50 for the flc.sample_flare_recovery() just for time's sake. Does that sound right to you?

Looking into your Google Colab notebook your results look correct. 16 +/- 2 or so flares is what you'd find in that GJ 1243 light curve. Nothing to do with sample_flare_recovery and the 50 iterations, though. The synthetic flares are stored in flc.fake_flares.

mrtommyb commented 3 years ago

Thanks for your response @ekaterinailin. I think that the tight integration with Lightkurve means that not having the software compatible with Lightkurve 2 means that the software is not entirely functional. I think that it's important to be able to use the current release of Lightkurve, without pinning the installation to an old and unmaintained version of Lightkurve.

dfm commented 3 years ago

Thanks @mrtommyb!

@ekaterinailin: I agree with Tom that lightkurve compatibility is pretty crucial since it's a major dependency, but also a common element in the workflow of many of your users. I think that it would be worth taking the time to look carefully into what it would take to support lightkurve 2+. Even better, how painful would it be to support both? If it's really infeasible, one option would be to add a prominent version disclaimer to the README and docs, and open an issue describing the steps that would be needed so that folks could contribute.

ekaterinailin commented 3 years ago

@mrtommyb @dfm Agreed! I have a few deadlines coming up, but I hope that - if feasible - I can make lightkurve2+ compatibility happen in early April. If that works out I also shall see if I can make the software work for both old and new lightkurve :+1:

dfm commented 3 years ago

@ekaterinailin: This sounds great and no rush! I think it'll be great to have this compatibility if possible. I'm going to add the paused label to this review for now and then you should just ping me when you are able to get back to it.

ekaterinailin commented 3 years ago

Hi @dfm, I finally got back to it. Not quite early April - but now AltaiPony 2.0. is available, test and notebooks seem to run with lightkurve2.0 :)

dfm commented 3 years ago

@ekaterinailin: Great to hear!

@mrtommyb: Are you be available to revisit your checklist now that we have compatibility with lightkurve 2?

mrtommyb commented 3 years ago

Hi @ekaterinailin, I've still working on a the examples, but one very quick thing is that in the Readme you have GitHub and email listed in several places that look like links but they don't link to anywhere.

ekaterinailin commented 3 years ago

Hi @ekaterinailin, I've still working on a the examples, but one very quick thing is that in the Readme you have GitHub and email listed in several places that look like links but they don't link to anywhere.

You're right, thanks! Fixed both links :)

mrtommyb commented 3 years ago

Sorry this took me forever to get finished. I'm very happy with the changes made. One thing you might want to look at (though I don't think it should slow down this paper), is that a number of warnings are produced related to calling deprecated functions in numpy (such as np.int, np.bool, etc.).

ekaterinailin commented 3 years ago

Thank you @mrtommyb ! I'll look into the warnings - can you recall what numpy version you were working with when you got the warnings?

dfm commented 3 years ago

Thanks @mrtommyb!

@ekaterinailin: Give me a day or two to do a final pass through the paper, etc. and then I'll have a few last administrative tasks for you before publication. Thanks for your patience!!

dfm commented 3 years ago

@ekaterinailin: I've opened a tiny pull request with an update for the references. 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!

ekaterinailin commented 3 years ago

@whedon generate pdf from branch josspaper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch josspaper. Reticulating splines etc...
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:

ekaterinailin commented 3 years ago

Thank you for updating and checking the references, @dfm ! I read through the paper - looks complete and correct to me.

The newest version of AltaiPony is now 2.0.1., and the corresponding DOI is 10.5281/zenodo.4095339

dfm commented 3 years ago

@whedon check references from branch josspaper

whedon commented 3 years ago 
Attempting to check references... from custom branch josspaper
whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1051/0004-6361/202039198 is OK
- 10.1051/0004-6361/201834400 is OK
- 10.1093/mnras/stw706 is OK
- 10.1086/670067 is OK
- 10.3847/0004-637X/829/1/23 is OK
- 10.1088/0004-637X/797/2/122 is OK
- 10.1093/mnras/staa2021 is OK
- 10.1111/j.1365-2966.2009.14577.x is OK
- 10.1086/421261 is OK
- 10.1088/2041-8205/713/2/L79 is OK
- 10.1086/676406 is OK
- 10.1117/1.JATIS.1.1.014003 is OK
- 10.21105/joss.02347 is OK

MISSING DOIs

- 10.3847/1538-4365/abe70e may be a valid DOI for title: Allesfitter: Flexible Star and Exoplanet Inference From Photometry and Radial Velocity

INVALID DOIs

- None
dfm commented 3 years ago

@whedon set 10.5281/zenodo.4095339 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.4095339 is the archive.

dfm commented 3 years ago

@whedon set v2.0.1 as version

whedon commented 3 years ago

OK. v2.0.1 is the version.

dfm commented 3 years ago

@ekaterinailin: Thanks!! I opened one last PR with a reference update - sorry about that! Once you merge that, we're good to go.

dfm commented 3 years ago

@whedon generate pdf from branch josspaper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch josspaper. Reticulating splines etc...
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 recommend-accept from branch josspaper

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.1051/0004-6361/202039198 is OK
- 10.1051/0004-6361/201834400 is OK
- 10.1093/mnras/stw706 is OK
- 10.1086/670067 is OK
- 10.3847/0004-637X/829/1/23 is OK
- 10.1088/0004-637X/797/2/122 is OK
- 10.1093/mnras/staa2021 is OK
- 10.1111/j.1365-2966.2009.14577.x is OK
- 10.1086/421261 is OK
- 10.1088/2041-8205/713/2/L79 is OK
- 10.3847/1538-4365/abe70e is OK
- 10.1086/676406 is OK
- 10.1117/1.JATIS.1.1.014003 is OK
- 10.21105/joss.02347 is OK

MISSING DOIs

- None

INVALID DOIs

- None