Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REVIEW]: sunraster: Manipulating and Visualizing Solar Slit-spectrograph Observations in Python #5318

Closed
editorialbot opened this issue Mar 31, 2023 · 27 comments
Assignees
Labels

Comments

@editorialbot
Copy link
Collaborator

editorialbot commented Mar 31, 2023

Submitting author: @DanRyanIrish (Daniel Ryan)
Repository: https://github.com/sunpy/sunraster
Branch with paper.md (empty if default branch):
Version: v0.4.3
Editor: @xuanxu
Reviewers: @mwcraig, @j-faria
Archive: Pending

Status

status

Status badge code:

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

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

@mwcraig & @j-faria, 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 @xuanxu 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 @j-faria

📝 Checklist for @mwcraig

@editorialbot editorialbot added Python review Shell TeX Track: 1 (AASS) Astronomy, Astrophysics, and Space Sciences labels Mar 31, 2023
@editorialbot
Copy link
Collaborator Author

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
Copy link
Collaborator Author

Software report:

github.com/AlDanial/cloc v 1.88  T=0.07 s (584.2 files/s, 71087.4 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          16            435            579           2067
reStructuredText                11            317            339            456
YAML                             6             20              4            221
Markdown                         1             12              0            122
TeX                              1              7              0             85
TOML                             1             11              0             73
INI                              1              3              0             54
DOS Batch                        1              8              1             26
make                             1              4              7              9
Bourne Shell                     1              1              0              6
-------------------------------------------------------------------------------
SUM:                            40            818            930           3119
-------------------------------------------------------------------------------


gitinspector failed to run statistical information for the repository

@editorialbot
Copy link
Collaborator Author

Wordcount for paper.md is 791

@editorialbot
Copy link
Collaborator Author

👉📄 Download article proof 📄 View article proof on GitHub 📄 👈

@editorialbot
Copy link
Collaborator Author

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1007/s01007-007-0293-1 is OK
- 10.1007/s11207-014-0485-y is OK
- 10.1051/0004-6361/201935574 is OK

MISSING DOIs

- None

INVALID DOIs

- None

@xuanxu
Copy link
Member

xuanxu commented Apr 3, 2023

👋 @mwcraig, @j-faria - Thanks for agreeing to review this submission.
This is the review thread for the paper. All of our communications will happen here from now on.

As you can see above, you each should use the command @editorialbot generate my checklist to create your review checklist.

We aim for reviews to be completed within about 2-4 weeks. Please let me know if either of you require some more time.
Reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#5318 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 to the author on this thread.

@xuanxu
Copy link
Member

xuanxu commented Apr 12, 2023

@mwcraig, @j-faria Please update us on how your reviews are going. You can create your checklist with the following command:
@editorialbot generate my checklist

@j-faria
Copy link

j-faria commented Apr 26, 2023

Review checklist for @j-faria

Conflict of interest

  • I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the https://github.com/sunpy/sunraster?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Contribution and authorship: Has the submitting author (@DanRyanIrish) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
  • Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
  • Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
  • Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
  • Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

@j-faria
Copy link

j-faria commented Apr 26, 2023

Hi @DanRyanIrish, I've now started the review of sunraster (apologies for the delay).
I have installed the package and went through the examples in the documentation; everything worked perfectly. In order to tick the remaining boxes, I have only one simple request and a couple of questions:

  • I'm missing from the documentation an example of using sunraster on a real-world analysis.
    Related to this, would it be possible to include with the package (or automatically fetch from somewhere else) an example dataset that is typical of what sunraster will be used for?
    Because this example is missing, I think the documentation also misses a figure or two. For example, the plot generated in this section could be included in the page? Admittedly, the plot is not very interesting because the data is just ones, but an example dataset would be more interesting.

  • I'm assuming the package is being tested continuously with this action, can you please confirm?

  • Are there other packages that are commonly used to achieve the same analysis that sunraster allows for?

@DanRyanIrish
Copy link

Hi @j-faria. Thanks so much for your review and for the positive feedback.

* I'm missing from the documentation an example of using `sunraster` on a real-world analysis.
  Related to this, would it be possible to include with the package (or automatically fetch from somewhere else) an example dataset that is typical of what `sunraster` will be used for?
  Because this example is missing, I think the documentation also misses a figure or two. For example, the plot generated [in this section](https://docs.sunpy.org/projects/sunraster/en/latest/data_types/spectrogram.html#plotting) could be included in the page? Admittedly, the plot is not very interesting because the data is just ones, but an example dataset would be more interesting.

Thanks for this helpful suggestion. I agree that this would be a helpful addition for users. I will give some thought as to the best way to implement it and let you know when it is ready to be viewed.

* I'm assuming the package is being tested continuously with [this action](https://github.com/sunpy/sunraster/actions/workflows/ci.yml), can you please confirm?

Yes that's right.

* Are there other packages that are commonly used to achieve the same analysis that `sunraster` allows for?

Regarding other packages that work with sunraster, much of the underlying infrastructure comes from the ndcube package. Updates in that package transparently become available through the sunraster data classes. For example, rebinning data into "macropixels", e.g. turning a 8x8 grid into a 4x4 grid by summing/averaging neighbouring 2x2 pixel areas, recently became available in ndcube. This functionality can now used used directly through the sunraster data classes and is often used in solar slit spectroscopy to boost signal-to-noise before performing spectral fits. Moreover, because the sunraster objects are compatible with the ndcube APIs, tools built on top of those APIs should work seamlessly with the sunraster data classes. Was this the question you were asking?

@mwcraig
Copy link

mwcraig commented May 1, 2023

Review checklist for @mwcraig

Conflict of interest

  • I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the https://github.com/sunpy/sunraster?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Contribution and authorship: Has the submitting author (@DanRyanIrish) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
  • Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
  • Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
  • Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
  • Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

@xuanxu
Copy link
Member

xuanxu commented May 17, 2023

@mwcraig @j-faria Can you give us an update on the state of your review?

@DanRyanIrish
Copy link

@xuanxu: I can acknowledge that @j-faria has requested additions to the documentation and asked whether an example data set can be provided somewhere. I have not yet implemented this. As far as I'm aware this is the only outstanding issue from review so far.

@xuanxu
Copy link
Member

xuanxu commented May 17, 2023

@DanRyanIrish Great, thanks!

@mwcraig
Copy link

mwcraig commented May 18, 2023

My sincere apologies for the long delay in completing my review.

Overall, this is a strong submission that would benefit, I think, from addressing these points:

  • Several of the builtin plots looked odd when I rendered them, and one failed. I've opened issues for the things I encountered in the sunraster repo.
  • All of the plots would benefit from the change that @j-faria suggested: add a more real-world example. Even if adding a truly realistic example isn't feasible because of the size of the data, amore interesting toy example would better demonstrate what sunraster accomplishes. Both the documentation and the paper would be improved with this.
  • The documentation is, overall, thorough. A few improvements are needed, though:
    • Add a prominent link to the project's code of conduct from the documentation.
    • Add information about how to contribute; a minimal approach here would copy/paste what is in the project's README
    • Consider adding a brief contributing page devoted just to sunraster that then links to the broader sunpy contributing guide. There are a couple of reasons for this, mostly related to testing:
      • It wasn't clear to me how I was supposed to run the tests locally, and whether I needed to pip install -e .\[test\] or whether the [dev] install included what I needed. I found that running pytest worked and the tests passed. Running sunraster.self_test() failed because there is no such function, even though that option is mentioned in the sunpy dev docs. Running tox failed with a bunch of errors like pass_env values cannot contain whitespace, use comma to have multiple values in a single line, invalid values found 'HOME WINDIR LC_ALL LC_CTYPE CC CI TRAVIS'
      • I was sort of confused about how to get started with contributing overall. Maybe at a minimum make sure sunraster is listed in the sunpy dev docs here https://docs.sunpy.org/en/latest/dev_guide/contents/newcomers.html#code
  • The paper didn't discuss whether there are other packages that provide similar functionality. My guess is that there are not, but a statement to that effect would be helpful.

@DanRyanIrish
Copy link

Thanks so much @mwcraig for this review. I will consider how to best address these comments. In the meantime, here are a few initial responses.

* Several of the builtin plots [looked odd when I rendered them](https://github.com/sunpy/sunraster/issues/241)

As highlighted on the issue thread, the root issue is that the WCS infrastructure in the astropy core package does not preserve the coordinate units but defaults everything to SI. However, perhaps there's a way of making the example code and built-in plots manually override this. This will make the docs nicer but won't prevent plots like this being produced by users in real-world analysis. But is might help to show users how to overcome this issue themselves.

and one failed.

This is an issue caused by changes to the latest releases of ndcube and needs addressing by sunraster. Thanks for spotting it.

* All of the plots would benefit from the change that @j-faria suggested: add a more real-world example. Even if adding a truly realistic example isn't feasible because of the size of the data, amore interesting toy example would better demonstrate what `sunraster` accomplishes. Both the documentation and the paper would be improved with this.

Thanks for highlighting the importance of this addition.

* The documentation is, overall, thorough. A few improvements are needed, though:

This feedback is very helpful as sometimes it requires a fresh pair of eyes to spot what's not clear.

* The paper didn't discuss whether there are other packages that provide similar functionality. My guess is that there are not, but a statement to that effect would be helpful.

There is no other Python package that specifically addresses the needs of handling data from solar slit spectrometers. This fact can be added to the paper.

@xuanxu
Copy link
Member

xuanxu commented Jul 7, 2023

Hey @DanRyanIrish, did you have the time to address the reviewers' comments? any update on the progress?

@DanRyanIrish
Copy link

Hi @xuanxu. Thanks for checking in on this. I have on yet completed addressing the reviewers comments but hope to have some more time after next week.

@DanRyanIrish
Copy link

DanRyanIrish commented Jul 8, 2023

Here I compile a list of action items emanating from the two reviews:

Paper Improvements

  • Mention that sunraster is unique in meeting the needs of solar slit spectrograph data analysis in Python.
  • Add a plot of solar slit spectrograph data being manipulated by sunraster.

Documentation Improvements

  • Add a real world example in the docs, including plots.
  • Set plots in docs to have readable (non-SI) ticklabels.
  • Add link to code of conduct
  • Add "How to Contribute" section. Include info on how to install testing envs and run tests.

@xuanxu
Copy link
Member

xuanxu commented Sep 29, 2023

@DanRyanIrish can you please provide an update or report your progress?

If you need more time I can pause the review until you are done with the proposed changes

@DanRyanIrish
Copy link

Hi @xuanxu. We have a plan for how to address the referee' comments that require another release of sunraster. Unfortunately due to other duties I have not had time to implement these changes and update the paper. I should have time to get back to this in about 6 weeks. Is it possible to pause the review for this amount of time?

@xuanxu
Copy link
Member

xuanxu commented Sep 29, 2023

Yes, I'll put this on hold for now.
Please let me know when we should restart it.

@xuanxu xuanxu added the paused label Sep 29, 2023
@xuanxu
Copy link
Member

xuanxu commented Dec 3, 2023

@DanRyanIrish do you want to keep the submission paused? any update on your side?

@DanRyanIrish
Copy link

Hi @xuanxu. Unfortunately, there hasn't been sufficient progress. I think it would therefore be best to withdraw this paper until further notice.

@xuanxu
Copy link
Member

xuanxu commented Dec 4, 2023

Hi @DanRyanIrish Sorry to hear that, hope you can resubmit sometime in the future.
Thank you @mwcraig and @j-faria for your reviews.

Pinging @openjournals/aass-eics for final withdraw

@dfm
Copy link

dfm commented Dec 4, 2023

@editorialbot withdraw

@DanRyanIrish — I'm sorry to hear that this is the outcome here!

@xuanxu, @mwcraig, @j-faria — Thank you all for the time that you committed to this review! JOSS runs on volunteer effort and we couldn't do this without all of you and it's very much appreciated!!

@editorialbot
Copy link
Collaborator Author

Paper withdrawn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants