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]: astrosource: automating optical astronomy measurement, calibration and analysis for variable stellar sources from provided photometry #2641

Closed whedon closed 3 years ago

whedon commented 4 years ago

Submitting author: @mfitzasp (Michael Fitzgerald) Repository: https://github.com/zemogle/astrosource/ Version: v1.5.2 Editor: @arfon Reviewer: @zackdraper, @joshspeagle Archive: 10.6084/m9.figshare.14113667.v1

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

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

@zackdraper & @joshspeagle, 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 @arfon 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 @zackdraper

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @joshspeagle

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. @bsipocz, @joshspeagle 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 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1080/21672857.2017.1303264 is OK
- 10.1017/pasa.2014.30 is OK
- 10.1002/asna.201512254 is OK
- 10.3847/1538-3881/153/2/77 is OK
- 10.1088/1538-3873/ab7ee7 is OK
- 10.1017/pasa.2018.5 is OK
- 10.1088/0067-0049/219/1/12 is OK
- 10.32374/atom.2020.1.2 is OK
- 10.32374/atom.2020.1.4 is OK
- 10.1117/12.2314340 is OK
- 10.1086/673168 is OK
- 10.1051/0004-6361/201322068 is OK
- 10.3847/1538-3881/aabc4f is OK
- 10.1109/mcse.2011.37 is OK
- 10.1109/mcse.2007.55 is OK
- 10.3847/1538-3881/aafc33 is OK
- 10.21105/joss.00058 is OK
- 10.32374/atom.2020.1.1 is OK
- 10.1051/0004-6361:20020802 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

arfon commented 4 years ago

@zackdraper, @joshspeagle

This is the review thread for the paper. All of our communications will happen here from now on.

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 https://github.com/openjournals/joss-reviews/issues/2641 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 reviews to be completed within about 2-4 weeks. Please let me know if any of you require some more time. We can also use Whedon (our bot) to set automatic reminders if you know you'll be away for a known period of time.

bsipocz commented 4 years ago

@arfon - in this the place to ask paper related questions (rather than software related)?

E.g. authorship related questions should be discussed here, for example the explicit opt in from authors, or in an issue in the repo? (I would this here is better, but there may be a guideline saying otherwise that I missed). Also, I didn't see any discussion in the guidelines about authors for whom there is no trace of contribution in the software repo. Or should the commit history on be used to determined the lead author has made significant contribution to the software?

danielskatz commented 4 years ago

These discussions can happen here or in issues in the repo (tagged with this issue to create a link between them).

bsipocz commented 4 years ago

... active project direction and other forms of non-code contributions are. The authors themselves assume responsibility for deciding who should be credited with co-authorship, and co-authors must always agree to be listed...

@mfitzasp - Since you and @zemogle interacted with the JOSS submission I take it as your agreement to be authors. I don't see any interactions in the repo with the other two authors though, have they been actively agreed to be listed?

Also, I wonder whether you considered adding the third person as author, who contributing to the package, especially early on helping with setting up the package skeleton, CI, etc. Unless they fully opted out, I would appreciate if they were at least mentioned in the acknowledgement.

danielskatz commented 4 years ago

@whedon check repository

whedon commented 4 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.84  T=0.19 s (158.9 files/s, 24758.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          18            584            376           2450
TeX                              1             25              0            278
Markdown                         2            114              0            191
reStructuredText                 3            101             21            170
Jupyter Notebook                 1              0            195             65
YAML                             2              3              2             40
DOS Batch                        1              8              1             26
make                             1              4              7              9
TOML                             1              0              0              3
-------------------------------------------------------------------------------
SUM:                            30            839            602           3232
-------------------------------------------------------------------------------

Statistical information for the repository '2641' was gathered on 2020/09/10.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Edward Gomez                    97          8541           5484           76.81
Joe Singleton                    3          1799           1765           19.52
Michael Fitzgerald              30           501            169            3.67

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
Edward Gomez               2913           34.1         10.0                7.83
Joe Singleton                69            3.8         14.0                1.45
Michael Fitzgerald          428           85.4          1.1                9.58
zemogle commented 4 years ago

That's a really good call. @joesingo would you like to be an author? You helped substantially with the tom_astrosource aspect as well as setting up RTD and Travis.

The last 2 authors provided development input before I was involved, and before the code was in GitHub.

zemogle commented 4 years ago

I've added @joesingo to the author list @bsipocz

arfon commented 4 years ago

:wave: @bsipocz & @joshspeagle - just wanted to check in to see how you're getting along with your reviews here?

bsipocz commented 4 years ago

@arfon - I got a bit swamped and dropped the ball, will try to get back to it over the weekend.

joshspeagle commented 4 years ago

Same situation here — I’ve been absolutely swamped but will get to it this weekend. Apologies for the delay.

joshspeagle commented 4 years ago

This will unfortunately be delayed even further on my end. I've recently moved internationally and, due to technical issues and quarantine requirements, it's taking longer to get internet set up at our new place than we had expected. I'm hopeful that this will be resolved in the next few days.

Sorry about the extensive delays getting this referee report in.

arfon commented 4 years ago

👋 @bsipocz & @joshspeagle - any chance you could try and complete your reviews in the next week or so?

joshspeagle commented 4 years ago

My review will be done by tomorrow. Thanks in advance to everyone for your patience.

joshspeagle commented 4 years ago

Review is done! Comments below:

As most of the above issues are recommendations and not requirements, I'm happy to recommend the submission be accepted.

zemogle commented 4 years ago

Thanks @joshspeagle Those are really constructive comments. The docs are an on-going effort but giving worked through examples are a good idea.

I've fixed up the figure references (and the missing captions).

zemogle commented 3 years ago

Any news on the status of this paper @arfon ?

arfon commented 3 years ago

Hi @zemogle - I emailed @bsipocz a few days ago to remind them about this review. I had meant to update this thread too when I did this (but forgot 🤦).

arfon commented 3 years ago

:wave: @zemogle - happy new year. I just sent a final reminder to @bsipocz. If I don't hear back from them this coming week then I'll go ahead and look for a new reviewer.

arfon commented 3 years ago

:wave: @gusbeane @zackdraper - would either of you be willing to review this submission for JOSS? We carry out our checklist-driven reviews here in GitHub issues and follow these guidelines: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html

The software under review here is astrosource: automating optical astronomy measurement, calibration and analysis for variable stellar sources from provided photometry which already has one complete review and we're looking for a second reviewer to replace @bsipocz who unfortunately isn't able to complete their review at this time.

gusbeane commented 3 years ago

Hi @arfon - thanks for the ping. Unfortunately the topic of this code is way out of my ballpark, and I'm a bit oversubscribed this semester (lots of coursework!). In the future I'd be willing to help review other code.

Good luck!

zackdraper commented 3 years ago

Sure, I'll give it a go over the weekend. Haven't done a JOSS review before but it looks straightforward.

arfon commented 3 years ago

@whedon re-invite @zackdraper as reviewer

whedon commented 3 years ago

OK, the reviewer has been re-invited.

@zackdraper please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

arfon commented 3 years ago

Sure, I'll give it a go over the weekend. Haven't done a JOSS review before but it looks straightforward.

⚡ thank you so much @zackdraper! Please review the instructions above, and work through the checklist at the top of the issue (you'll need to accept the repository invite above from Whedon before you can update the checklist).

Any questions, don't hesitate to ask!

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

zackdraper commented 3 years ago

@zemogle Do you have a link to an example data set to use? Trying to confirm functionality on real data. Tried getting public data off of LCO and got a few errors.

The error for a missing keyword was caught, but I would suggest having documentation saying what fits header keywords are required for the input data.

I got another error resulting from a malformatted .fits file on "hdulist[2]" giving a Python KeyError.

zemogle commented 3 years ago

Hey @zackdraper 👋 Thanks for picking this up. Just picking a random data set, you could try this data set of Tres 2b.

The coords of Tres 2b are --ra 286.8083 --dec 49.3164 if you need them. I've just just tried astrosource with this dataset using the --full option and left it. All worked as expected.

I think the error you mentioned might have been from a fits file which doesn't have the photometry data in the fits extension.

Good catch on the documentation. As with all documentation it is a work in progress. I've never written documentation before, so any and all suggestions are helpful.

zackdraper commented 3 years ago

Hey @zemogle I ran it on the Tres2b data and got an error, looks like there needs to be more exception handling when intermediate steps don't return what is normally expected.

(py3) zackdraper@zackdraper-MacBookAir:~/JOSS$ astrosource --ra 286.8083 --dec 49.3164 --indir lcogtdata-20210211-10/ --full
💾 Inspecting input files
🌟 Identify comparison stars for photometry calculations
..........
⭐️ Find stable comparison stars for differential photometry
..................................................................................................................................................................................................💫.................................................................................................................................................................................................💫...............................................................................................................................................................................................
⭐️ Find comparison stars in catalogues for calibrated photometry
..........
Traceback (most recent call last):
  File "/home/zackdraper/anaconda2/envs/py3/bin/astrosource", line 11, in <module>
    sys.exit(main())
  File "/home/zackdraper/anaconda2/envs/py3/lib/python3.6/site-packages/click/core.py", line 722, in __call__
    return self.main(*args, **kwargs)
  File "/home/zackdraper/anaconda2/envs/py3/lib/python3.6/site-packages/click/core.py", line 697, in main
    rv = self.invoke(ctx)
  File "/home/zackdraper/anaconda2/envs/py3/lib/python3.6/site-packages/click/core.py", line 895, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/zackdraper/anaconda2/envs/py3/lib/python3.6/site-packages/click/core.py", line 535, in invoke
    return callback(*args, **kwargs)
  File "/home/zackdraper/anaconda2/envs/py3/lib/python3.6/site-packages/astrosource/main.py", line 90, in main
    ts.find_variables()
  File "/home/zackdraper/anaconda2/envs/py3/lib/python3.6/site-packages/astrosource/astrosource.py", line 69, in find_variables
    find_variable_stars(targets=self.targets, parentPath=self.paths['parent'])
  File "/home/zackdraper/anaconda2/envs/py3/lib/python3.6/site-packages/astrosource/analyse.py", line 175, in find_variable_stars
    plot_variability(outputVariableHolder, parentPath)
  File "/home/zackdraper/anaconda2/envs/py3/lib/python3.6/site-packages/astrosource/plots.py", line 74, in plot_variability
    outplotx = asarray(output)[:, 2]
IndexError: too many indices for array: array is 1-dimensional, but 2 were indexed
zemogle commented 3 years ago

Hmmm. This is puzzling. I just made a new environment and installed astrosource from pypi. Everything ran smoothly. I also did a checkout of master to see if there was an issue there, but it also ran smoothly. None of our other users have reported this problem either so I'm finding it a bit tricky to debug.

Would you be able to make an issue so we keep this separate from the paper review ?

zackdraper commented 3 years ago

I have to verify the code actually works. Do you have another data set of an actual variable star? I think at the very least you would need to document an example dataset people can use. Obviously cant include it because the data would be too large, but some public data source is useful for this code to be tested with. Also, the fits files require certain keywords to work, I think you need to document those requirements as well. Imagine someone tries this on another observatory dataset and it happens to use non-standard fits headers. This would make it more useful for the amateur's you are trying to help with this software.

I followed a fresh install as documented, but I'll try it on another computer just to be sure.

I appreciate that this review process has gone on for a very long time for you and hope to get it finished soon.

zemogle commented 3 years ago

I think the docs should be clearer on this but astrosource doesn't do the photometric extraction, it just does the photometric analysis. It really only operates on FITS files which have a photometry table extension. So the inclusion of FITS format is just for data tables not images (see Basic Usage part of the docs). The exact columns needed are also detailed in that section.

Here is a small public dataset of variable star RX Eri. You'll need to use the following options with it because these are .psx format not FITS Table. 

--ra 72.43451
--dec -15.74109
--format psx

If that doesn't work, I suspect there is something about the versions of NumPy or Matplotlib which is causing the shenanigens. It would be really helpful if you could post an issue, so we can track the problem separately to the paper review. As I understand it the paper review should be higher level than code bugs.

Thanks for bearing with us!

zackdraper commented 3 years ago

Ok, I'm satisfied that the code is functional now with that example. But, I would strongly encourage you to include an actual example like the RX Eri data set somewhere in the documentation. You have a few examples shown in the paper. Ideally, for open source software, I would think those examples should be readily available to all users to immitate and prepare thier own data. astrosource --help also needs better documentation on what the parameters are (for ease of usabaility, the parameters are doccumented suffeciently in the readme). The exception handling issues can be chalked up to bugs to be fixed, but usable examples are really how others are going to take this open source code as a starting point to do thier own work, modify the code, etc.

zackdraper commented 3 years ago

@arfon My review is done now (I think). I'll recommend it's accepted.

zemogle commented 3 years ago

Thanks @zackdraper and @joshspeagle for very constructive reviews. We'll find a way to include a useful dataset and add more complete examples to the docs 👍

arfon commented 3 years ago

@mfitzasp @zemogle - At this point could you make a new release of this software that includes the changes that have resulted from this review. Then, please make an archive of the software in Zenodo/figshare/other service and update this thread with the DOI of the archive? For the Zenodo/figshare archive, please make sure that:

I can then move forward with accepting the submission.

zemogle commented 3 years ago

I've updated the code with the suggestions, published on PyPi and made a release which is on Figshare. Here is the DOI

I think that's it! Let me know if need anything else for publication. This has been a really great experience! 👍

arfon commented 3 years ago

@whedon set 10.6084/m9.figshare.14113667.v1 as archive

whedon commented 3 years ago

OK. 10.6084/m9.figshare.14113667.v1 is the archive.

arfon commented 3 years ago

@whedon 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.1080/21672857.2017.1303264 is OK
- 10.1017/pasa.2014.30 is OK
- 10.1002/asna.201512254 is OK
- 10.3847/1538-3881/153/2/77 is OK
- 10.1088/1538-3873/ab7ee7 is OK
- 10.1017/pasa.2018.5 is OK
- 10.1088/0067-0049/219/1/12 is OK
- 10.32374/atom.2020.1.2 is OK
- 10.32374/atom.2020.1.4 is OK
- 10.1117/12.2314340 is OK
- 10.1086/673168 is OK
- 10.1051/0004-6361/201322068 is OK
- 10.3847/1538-3881/aabc4f is OK
- 10.1109/mcse.2011.37 is OK
- 10.1109/mcse.2007.55 is OK
- 10.3847/1538-3881/aafc33 is OK
- 10.21105/joss.00058 is OK
- 10.32374/atom.2020.1.1 is OK
- 10.1051/0004-6361:20020802 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/2113

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

@whedon accept deposit=true
kthyng commented 3 years ago

Huh, I don't see figshare come through too often so I might be looking in the wrong place, but I am having trouble verifying that the title and author list match the JOSS submission. @zemogle can you point me to where this is, or update it on your end?

kthyng commented 3 years ago

@zemogle can you verify what version of your software should be associated with this review?

zemogle commented 3 years ago

@kthyng If you click the DOI link you should be able to see the title and authors list (Figshare shows a preview of the archive first, bizarrely).

The version matching the paper is 1.5.2