search

openjournals / joss-reviews

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

[REVIEW]: CR-Sparse: Hardware accelerated functional algorithms for sparse signal processing in Python using JAX #3917

Closed whedon closed 2 years ago

whedon commented 2 years ago

Submitting author: @shailesh1729 (Shailesh Kumar) Repository: https://github.com/carnotresearch/cr-sparse Version: v0.2.2 Editor: @pdebuyl Reviewer: @Saran-nns, @mirca Archive: 10.5281/zenodo.5749792

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

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 & @mirca, 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 @pdebuyl 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 @Saran-nns

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @mirca

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 2 years ago

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

Wordcount for paper.md is 4567

whedon commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1145/3088525.3088527 is OK
- 10.1109/ICCV.2001.937651 is OK
- 10.1016/j.acha.2009.04.002 is OK
- 10.1109/jstsp.2010.2042411 is OK
- 10.1109/wvm.1991.212809 is OK
- 10.1023/A:1008000628999 is OK
- 10.1109/tit.2009.2016006 is OK
- 10.1109/tit.2010.2054653 is OK
- 10.1007/978-1-4419-7011-4 is OK
- 10.1109/cvprw.2009.5206547 is OK
- 10.1109/tpami.2013.57 is OK
- 10.1109/CVPR.2003.1211332 is OK
- 10.1109/iccv.2001.937679 is OK
- 10.1109/tpami.2005.92 is OK
- 10.21105/joss.01237 is OK
- 10.1016/b978-0-12-374370-1.x0001-8 is OK
- 10.1016/j.acha.2008.07.002 is OK
- 10.1109/acssc.1993.342465 is OK
- 10.1109/34.584098 is OK
- 10.1007/bf00129684 is OK
- 10.1175/1520-0477(1998)079<0061:APGTWA>2.0.CO;2 is OK
- 10.1109/TIT.2004.834793 is OK
- 10.1109/MSP.2010.939739 is OK
- 10.1109/CVPR.2016.425 is OK
- 10.1007/s11263-012-0535-6 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 2 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.36 s (1049.5 files/s, 132232.0 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
Python                          295           4312           8807          16578
TeX                               2            484              1           3263
reStructuredText                 54           1291           1502           1047
Markdown                          7            200              0            894
Jupyter Notebook                  9              0           8309            616
YAML                              4              9              7            148
JavaScript                        1              6             16            117
CSS                               1              9              0             37
JSON                              2              0              0             33
DOS Batch                         1              8              1             26
make                              1              5              7             13
Bourne Again Shell                1              2              0              3
Bourne Shell                      1              1              0              2
--------------------------------------------------------------------------------
SUM:                            379           6327          18650          22777
--------------------------------------------------------------------------------

Statistical information for the repository 'e94c8f59a0f0e97d97a5b8ce' was
gathered on 2021/11/16.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Shailesh Kumar                 587         33404           3399          100.00

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
Shailesh Kumar            30005           89.8          2.9               22.73
whedon commented 2 years ago

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

pdebuyl commented 2 years ago

@Saran-nns @mirca make sure to accept the invitation to the reviewers group and to have a look at the reviewer guidelines linked to at the top of this review page.

@shailesh1729 the article is very long by JOSS standard, I recommed to shorten it already. The explanations about l0 and l1 problems and about the linear operators are best left in the software documentation.

shailesh1729 commented 2 years ago

@pdebuyl, thanks for the input. Let me work on shortening it.

shailesh1729 commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

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

shailesh1729 commented 2 years ago

@pdebuyl, I have revised the paper and moved the theoretical explanations to the online documentation and placed references to it in the paper draft. It is down to 7 pages in draft.

Kindly, let me know if I should work on shortening it further.

With regards, Shailesh

Saran-nns commented 2 years ago

Hi, a quick update. I created an issue with some suggestions at the repository.

pdebuyl commented 2 years ago

Hi @shailesh1729 thanks for the editing, this is better. The listing of features in the summary is still excessive (explicit and detailed listing of all subpackages is also for the documentation). Also, a mention such as "For details, see the onlinedocumentation" is redundant.

shailesh1729 commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

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

shailesh1729 commented 2 years ago

HI @Saran-nns, I have prepared a new draft based on your feedback in carnotresearch/cr-sparse#21. Could you please see if it addresses your concerns reasonably? I have provided more clarifications on the issue itself.

Hi @pdebuyl, thank you again for your feedback. I have tried to shorten the package overview in this revision. I will revise it again after I close the review feedback loop with @Saran-nns.

Saran-nns commented 2 years ago

@shailesh1729 Besides the concern over the length of the draft as noted by @pdebuyl, I am convinced with the rest. I closed the issue https://github.com/carnotresearch/cr-sparse/issues/21 in favor of @shailesh1729

shailesh1729 commented 2 years ago

@Saran-nns, I have resolved carnotresearch/cr-sparse#22. Kindly check.

mirca commented 2 years ago

@shailesh1729 I'm happy with the current state of the library/paper apart from the issues I have opened in the repo. Please, take a look.

shailesh1729 commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

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

shailesh1729 commented 2 years ago

@mirca, thanks a lot for your feedback. I have updated the online docs and the paper based on the issues you filed. Kindly check. Let me know if there is anything else missing.

Saran-nns commented 2 years ago

@pdebuyl My checklist is finished. It is my pleasure to review this package and I am happy to recommend 💯 accept. Well done @shailesh1729 :)

mirca commented 2 years ago

@pdebuyl I have completed my review and I'm happy to recommend this package for acceptance in JOSS. Thanks @shailesh1729!

shailesh1729 commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

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

shailesh1729 commented 2 years ago

Thank you @Saran-nns and @mirca for your kind feedback.

@pdebuyl I have done one more round of editing and brought down the paper to 7 pages while keeping all the changes and additions suggested by the reviewers. Kindly, let me know if this is good enough.

pdebuyl commented 2 years ago

Thanks all for the very proactive work here! I'll proceed to the editorial steps during the week.

pdebuyl commented 2 years ago

Hi @shailesh1729

I went through the papers and have minor comments:

If the code has not changed since release v0.2.1 https://zenodo.org/record/5635717 I'll use that as the archive, can you confirm?

The new version of the article is much better in terms of length, thanks!

shailesh1729 commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

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

shailesh1729 commented 2 years ago

HI @pdebuyl,

Thank you for the detailed feedback. I have corrected the bibliographic and other editing issues in the new draft.

I have created the zenodo archive for the companion site:

I have cited the second one in the paper.

During the last month of review, while the code didn't change, the documentation and README were revised significantly based on the feedback received here. Hence, I have created a new release (v0.2.2) just now capturing all the doc updates.

The Zenodo archive link for v0.2.2 is: https://doi.org/10.5281/zenodo.5749792 This should be used for archival purposes.

Hope this addresses the concerns listed by you.

Thank you again for all your help in improving the quality of the software and the paper.

pdebuyl commented 2 years ago

Thank you for the update, all is good on my side!

Thanks @Saran-nns and @mirca for the review!

pdebuyl commented 2 years ago

@whedon set v0.2.2 as version

whedon commented 2 years ago

OK. v0.2.2 is the version.

pdebuyl commented 2 years ago

@whedon set 10.5281/zenodo.5749792 as archive

whedon commented 2 years ago

OK. 10.5281/zenodo.5749792 is the archive.

pdebuyl commented 2 years ago

@whedon recommend-accept

whedon commented 2 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1145/3088525.3088527 is OK
- 10.1109/ICCV.2001.937651 is OK
- 10.1137/080716542 is OK
- 10.1007/s12532-011-0029-5 is OK
- 10.1016/j.acha.2009.04.002 is OK
- 10.1109/jstsp.2010.2042411 is OK
- 10.1109/wvm.1991.212809 is OK
- 10.1016/j.crma.2008.03.014 is OK
- 10.1137/S003614450037906X is OK
- 10.1023/A:1008000628999 is OK
- 10.1109/tit.2009.2016006 is OK
- 10.1002/cpa.20042 is OK
- 10.1109/tit.2010.2054653 is OK
- 10.1109/tit.2006.871582 is OK
- 10.1007/978-1-4419-7011-4 is OK
- 10.1109/cvprw.2009.5206547 is OK
- 10.1109/tpami.2013.57 is OK
- 10.1007/978-0-8176-4948-7 is OK
- 10.1109/CVPR.2003.1211332 is OK
- 10.1109/iccv.2001.937679 is OK
- 10.1109/JSTSP.2007.910971 is OK
- 10.1109/tpami.2005.92 is OK
- 10.21105/joss.01237 is OK
- 10.1016/b978-0-12-374370-1.x0001-8 is OK
- 10.1109/ACCESS.2018.2886471 is OK
- 10.1016/j.acha.2008.07.002 is OK
- 10.1561/2400000003 is OK
- 10.1109/acssc.1993.342465 is OK
- 10.1109/34.584098 is OK
- 10.1016/j.sigpro.2020.107548 is OK
- 10.1109/JCN.2013.000083 is OK
- 10.5281/zenodo.5749656 is OK
- 10.21105/joss.02844 is OK
- 10.1007/bf00129684 is OK
- 10.1175/1520-0477(1998)079<0061:APGTWA>2.0.CO;2 is OK
- 10.1109/TIT.2004.834793 is OK
- 10.1109/MSP.2010.939739 is OK
- 10.1137/090777761 is OK
- 10.1109/CVPR.2016.425 is OK
- 10.1007/s11263-012-0535-6 is OK

MISSING DOIs

- 10.1145/355984.355989 may be a valid DOI for title: LSQR: An algorithm for sparse linear equations and sparse least squares

INVALID DOIs

- None
whedon commented 2 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/2791

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/2791, 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 2 years ago

Hi @shailesh1729! I'll be helping with final publishing.

Can you update the metadata on your Zenodo archive so the title and author list exactly match your JOSS paper?

kthyng commented 2 years ago

Paper and version look good.

shailesh1729 commented 2 years ago

Thanks, @kthyng,

I just updated the metadata (title and author list) on the Zenodo archive https://doi.org/10.5281/zenodo.5749792

Does this look good? Anything else that I need to do?

pdebuyl commented 2 years ago

@kthyng "metadata on your Zenodo archive so the title and author list exactly match your JOSS paper" -> sorry, I forgot to check it

pdebuyl commented 2 years ago

and the LSQR entry from the TOMS does not have a DOI to avoid you looking for it :-)

shailesh1729 commented 2 years ago

@whedon check references

whedon commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1145/3088525.3088527 is OK
- 10.1109/ICCV.2001.937651 is OK
- 10.1137/080716542 is OK
- 10.1007/s12532-011-0029-5 is OK
- 10.1016/j.acha.2009.04.002 is OK
- 10.1109/jstsp.2010.2042411 is OK
- 10.1109/wvm.1991.212809 is OK
- 10.1016/j.crma.2008.03.014 is OK
- 10.1137/S003614450037906X is OK
- 10.1023/A:1008000628999 is OK
- 10.1109/tit.2009.2016006 is OK
- 10.1002/cpa.20042 is OK
- 10.1109/tit.2010.2054653 is OK
- 10.1109/tit.2006.871582 is OK
- 10.1007/978-1-4419-7011-4 is OK
- 10.1109/cvprw.2009.5206547 is OK
- 10.1109/tpami.2013.57 is OK
- 10.1007/978-0-8176-4948-7 is OK
- 10.1109/CVPR.2003.1211332 is OK
- 10.1109/iccv.2001.937679 is OK
- 10.1109/JSTSP.2007.910971 is OK
- 10.1109/tpami.2005.92 is OK
- 10.21105/joss.01237 is OK
- 10.1016/b978-0-12-374370-1.x0001-8 is OK
- 10.1109/ACCESS.2018.2886471 is OK
- 10.1016/j.acha.2008.07.002 is OK
- 10.1145/355984.355989 is OK
- 10.1561/2400000003 is OK
- 10.1109/acssc.1993.342465 is OK
- 10.1109/34.584098 is OK
- 10.1016/j.sigpro.2020.107548 is OK
- 10.1109/JCN.2013.000083 is OK
- 10.5281/zenodo.5749656 is OK
- 10.21105/joss.02844 is OK
- 10.1007/bf00129684 is OK
- 10.1175/1520-0477(1998)079<0061:APGTWA>2.0.CO;2 is OK
- 10.1109/TIT.2004.834793 is OK
- 10.1109/MSP.2010.939739 is OK
- 10.1137/090777761 is OK
- 10.1109/CVPR.2016.425 is OK
- 10.1007/s11263-012-0535-6 is OK

MISSING DOIs

- None

INVALID DOIs

- None
shailesh1729 commented 2 years ago

It was my mistake. I added the LSQR reference later and forgot to add the DOI.

shailesh1729 commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

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