openjournals / joss-reviews

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

[REVIEW]: SLEPLET: Slepian scale-discretised wavelets in Python #5221

Closed editorialbot closed 1 year ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@paddyroddy<!--end-author-handle-- (Patrick James Roddy) Repository: https://github.com/astro-informatics/sleplet Branch with paper.md (empty if default branch): main Version: v1.3.6 Editor: !--editor-->@vissarion<!--end-editor-- Reviewers: @Saran-nns, @klb2 Archive: 10.5281/zenodo.7835860

Status

status

Status badge code:

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

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

@Saran-nns & @klb2, 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 @vissarion 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 @klb2

📝 Checklist for @Saran-nns

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.18 s (918.4 files/s, 65529.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                         140           1613           1363           7221
Markdown                         3            134              0            571
TOML                             8             10              0            265
YAML                             5             21              0            158
TeX                              1              0              0            120
Unity-Prefab                     4              0              0             12
-------------------------------------------------------------------------------
SUM:                           161           1778           1363           8347
-------------------------------------------------------------------------------

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

Wordcount for paper.md is 968

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

OK DOIs

- 10.1002/j.1538-7305.1961.tb03977.x is OK
- 10.1002/j.1538-7305.1962.tb03279.x is OK
- 10.1051/0004-6361/201220729 is OK
- 10.1145/3134472.3134497 is OK
- 10.1109/TSP.2011.2166394 is OK
- 10.5281/zenodo.7268074 is OK
- 10.1109/LSP.2021.3050961 is OK
- 10.1109/TSP.2022.3233309 is OK
- 10.5281/zenodo.4085210 is OK
- 10.1002/j.1538-7305.1961.tb03976.x is OK
- 10.1111/j.1365-2966.2008.13448.x is OK
- 10.1029/2018GC007529 is OK

MISSING DOIs

- None

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:

klb2 commented 1 year ago

Review checklist for @klb2

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Saran-nns commented 1 year ago

Review checklist for @Saran-nns

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

klb2 commented 1 year ago

I currently have issues installing the required packages pyssht and pys2let on my machine. I have opened a bug report in their projects.

After I am able to resolve these issues, I will continue my review of the submitted sleplet package.

klb2 commented 1 year ago

The developers of the pyssht and pys2let packages fixed the issue and I was now able to install the sleplet package.

I will continue my review this week.

klb2 commented 1 year ago

From the JOSS guidelines (https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain):

As this short list shows, JOSS papers are only expected to contain a limited set of metadata (see example below), a Statement of need, Summary, Acknowledgements, and References sections. You can look at an example accepted paper. Given this format, a “full length” paper is not permitted, and software documentation such as API (Application Programming Interface) functionality should not be in the paper and instead should be outlined in the software documentation.

Based on this, I think that the "Demonstration" section should be completely removed from the paper and the "Conclusions" section should be merged into the "Summary" and "Statement of Needs" sections.

paddyroddy commented 1 year ago

From the JOSS guidelines (https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain):

As this short list shows, JOSS papers are only expected to contain a limited set of metadata (see example below), a Statement of need, Summary, Acknowledgements, and References sections. You can look at an example accepted paper. Given this format, a “full length” paper is not permitted, and software documentation such as API (Application Programming Interface) functionality should not be in the paper and instead should be outlined in the software documentation.

Based on this, I think that the "Demonstration" section should be completely removed from the paper and the "Conclusions" section should be merged into the "Summary" and "Statement of Needs" sections.

Have done

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

klb2 commented 1 year ago

Is there any documentation besides the examples in the README? I could not find an API documentation that documents all of the individual functions of the package.

klb2 commented 1 year ago

In the paper, it is stated that

To the author’s knowledge, there is no public software which allows one to compute Slepian wavelets (or similar approach) on the sphere or general manifolds/meshes. SHTools (Wieczorek & Meschede, 2018) and slepian_alpha (Simons et al., n.d.) are examples of codes which allow one to compute Slepian functions on the sphere. In conjunction with SSHT (McEwen & Wiaux, 2011), S2LET (Leistedt, B. et al., 2013) may be used to develop scale-discretised wavelets on the whole sphere.

and later

Whilst Slepian wavelets may be trivially computed from a set of Slepian functions, the computation of the spherical Slepian functions themselves are computationally complex

Based on this, it is unclear for me what the advantages of the sleplet package are over the existing ones like SHTools and slepian_alpha. It is said in the paper that these two packages allow computing the Slepian functions and that it is trivial to compute the Slepian wavelets from the Slepian functions.

vissarion commented 1 year ago

Good point @klb2. @paddyroddy could you please clarify?

paddyroddy commented 1 year ago

On closer inspection ShTools only computes the Slepian functions of a polar cap https://shtools.github.io/SHTOOLS/pysphericalcapcoef.html which is much easier to compute. Whereas SLEPLET handles any arbitrary mask. Further, it works for mesh data rather than just the sphere.

slepian_alpha is a huge set of MATLAB code with no documentation or anything. I believe it's largely used to create the various plots on this paper, https://arxiv.org/pdf/math/0408424.pdf. Again, it is defined only for the sphere.

In an ideal world, SLEPLET would have relied on another library to generate the Slepian functions and the build the wavelets from them. However, this proved non-trivial to use and, as such, it was necessary to compute them myself. My wording should have been clearer.

paddyroddy commented 1 year ago

Is there any documentation besides the examples in the README? I could not find an API documentation that documents all of the individual functions of the package.

I hadn't realised full API documentation was required. I can look into this.

paddyroddy commented 1 year ago

Is there any documentation besides the examples in the README? I could not find an API documentation that documents all of the individual functions of the package.

I hadn't realised full API documentation was required. I can look into this.

I've made some: https://astro-informatics.github.io/sleplet/

klb2 commented 1 year ago

On closer inspection ShTools only computes the Slepian functions of a polar cap https://shtools.github.io/SHTOOLS/pysphericalcapcoef.html which is much easier to compute. Whereas SLEPLET handles any arbitrary mask. Further, it works for mesh data rather than just the sphere.

slepian_alpha is a huge set of MATLAB code with no documentation or anything. I believe it's largely used to create the various plots on this paper, https://arxiv.org/pdf/math/0408424.pdf. Again, it is defined only for the sphere.

In an ideal world, SLEPLET would have relied on another library to generate the Slepian functions and the build the wavelets from them. However, this proved non-trivial to use and, as such, it was necessary to compute them myself. My wording should have been clearer.

Thank you for clarifying. I guess it would be good, if some of these details could also be added to the paper in order to make the differences to the sleplet package clear.

klb2 commented 1 year ago

With the latest version of the software, I was able to run all provided examples without any errors. The unit tests also all pass on my machine. Thank you for adding pooch for the data management/downloads @paddyroddy.

klb2 commented 1 year ago

From my side as a reviewer, only the following points are open:

paddyroddy commented 1 year ago

I've added argument/function level description of the API https://astro-informatics.github.io/sleplet/index.html

paddyroddy commented 1 year ago

Have addressed the community guidelines

paddyroddy commented 1 year ago

On closer inspection ShTools only computes the Slepian functions of a polar cap https://shtools.github.io/SHTOOLS/pysphericalcapcoef.html which is much easier to compute. Whereas SLEPLET handles any arbitrary mask. Further, it works for mesh data rather than just the sphere. slepian_alpha is a huge set of MATLAB code with no documentation or anything. I believe it's largely used to create the various plots on this paper, https://arxiv.org/pdf/math/0408424.pdf. Again, it is defined only for the sphere. In an ideal world, SLEPLET would have relied on another library to generate the Slepian functions and the build the wavelets from them. However, this proved non-trivial to use and, as such, it was necessary to compute them myself. My wording should have been clearer.

Thank you for clarifying. I guess it would be good, if some of these details could also be added to the paper in order to make the differences to the sleplet package clear.

I've added some comments to explain this.

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

klb2 commented 1 year ago

On closer inspection ShTools only computes the Slepian functions of a polar cap https://shtools.github.io/SHTOOLS/pysphericalcapcoef.html which is much easier to compute. Whereas SLEPLET handles any arbitrary mask. Further, it works for mesh data rather than just the sphere. slepian_alpha is a huge set of MATLAB code with no documentation or anything. I believe it's largely used to create the various plots on this paper, https://arxiv.org/pdf/math/0408424.pdf. Again, it is defined only for the sphere. In an ideal world, SLEPLET would have relied on another library to generate the Slepian functions and the build the wavelets from them. However, this proved non-trivial to use and, as such, it was necessary to compute them myself. My wording should have been clearer.

Thank you for clarifying. I guess it would be good, if some of these details could also be added to the paper in order to make the differences to the sleplet package clear.

I've added some comments to explain this.

Thank you @paddyroddy for adding this part to the paper. It all looks good for me now. The only minor comment: The dates/years for two references are missing in the paper, for the SLEPLET package on Zenodo (2023) and for the slepian_alpha package on Zenodo (2020).

klb2 commented 1 year ago

@vissarion: I have completed my review and after @paddyroddy has added the missing years to the references in the paper, I recommend the paper for acceptance.

paddyroddy commented 1 year ago

Years added!

paddyroddy commented 1 year ago

@editorialbot generate preprint

editorialbot commented 1 year ago

:page_facing_up: Preprint file created: Find it here in the Artifacts list :page_facing_up:

Saran-nns commented 1 year ago

@paddyroddy I started my review and created an issue in the repo with few suggestions and requests

klb2 commented 1 year ago

Years added!

Thank you @paddyroddy

That completes my review @vissarion

Saran-nns commented 1 year ago

@vissarion @paddyroddy Couple of steps to go. Will be happy to flag accept then

Saran-nns commented 1 year ago

@paddyroddy thank you, great work @vissarion my checklist is complete and I am happy to recommend accept

paddyroddy commented 1 year ago

@editorialbot generate preprint

paddyroddy commented 1 year ago

Thank you, @klb2 and @Saran-nns for your guidance.

editorialbot commented 1 year ago

:page_facing_up: Preprint file created: Find it here in the Artifacts list :page_facing_up:

paddyroddy commented 1 year ago

Sorry just have just capitalised the title

paddyroddy commented 1 year ago

@editorialbot generate preprint

vissarion commented 1 year ago

Many thanks to both reviewers @Saran-nns and @klb2 !

@paddyroddy could you please create a tagged release and an archive with the code (e.g. in zenodo) and share the link and the release version here? Please use the same title and authors while creating the zenodo archive.

paddyroddy commented 1 year ago

@vissarion here is the archive link https://zenodo.org/record/7802428, current version v1.3.4

vissarion commented 1 year ago

Thanks! Please use the full title in the archive i.e. "SLEPLET: Slepian scale-discretised wavelets in Python" which is the same as the paper.

vissarion commented 1 year ago

@editorialbot generate pdf

vissarion commented 1 year ago

@editorialbot check references

editorialbot commented 1 year ago

Couldn't check the bibtex because branch name is incorrect: joss-paper

editorialbot commented 1 year ago

:warning: An error happened when generating the pdf.

vissarion commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

:warning: An error happened when generating the pdf.