openjournals / joss-reviews

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

[REVIEW]: Scientific Computational Imaging Code (SCICO) #4722

Closed editorialbot closed 1 year ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@bwohlberg<!--end-author-handle-- (Brendt Wohlberg) Repository: https://github.com/lanl/scico Branch with paper.md (empty if default branch): joss-paper Version: v0.0.3 Editor: !--editor-->@danielskatz<!--end-editor-- Reviewers: @vitorsr, @DanNixon, @lucaferranti Archive: 10.5281/zenodo.7255839

Status

status

Status badge code:

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

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

@vitorsr & @DanNixon & @lucaferranti, 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 @danielskatz 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 @vitorsr

πŸ“ Checklist for @DanNixon

πŸ“ Checklist for @lucaferranti

editorialbot commented 2 years 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 2 years ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.23 s (789.6 files/s, 135795.9 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                         133           4946           6265          14317
reStructuredText                24           1028            450           1833
TeX                              2             67              0            789
Bourne Shell                     7             82             69            402
SVG                              1              0              0            396
YAML                            11             45             75            374
make                             2             29              6            151
CSS                              1             50             10            130
Markdown                         1             25              0             64
TOML                             1              3              1             31
INI                              1              0              0              5
-------------------------------------------------------------------------------
SUM:                           184           6275           6876          18492
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago

Wordcount for paper.md is 765

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

OK DOIs

- 10.1137/080716542 is OK
- 10.1137/1.9781611974997 is OK
- 10.1561/2200000016 is OK
- 10.1007/s10851-010-0251-1 is OK
- 10.1117/12.766355 is OK
- 10.1002/cpa.20042 is OK
- 10.1137/09076934x is OK
- 10.1016/0898-1221(76)90003-1 is OK
- 10.1109/TIP.2017.2713099 is OK
- 10.1109/LSP.2017.2763583 is OK
- 10.1109/MSP.2020.3016905 is OK
- 10.1007/978-0-387-40065-5 is OK
- 10.1561/2400000003 is OK
- 10.1561/2400000003 is OK
- 10.1109/iccv.2011.6126441 is OK
- 10.1109/TCI.2016.2599778 is OK
- 10.1109/GlobalSIP.2013.6737048 is OK
- 10.1090/s0025-5718-2012-02598-1 is OK
- 10.1109/TIP.2017.2662206 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot 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:

danielskatz commented 2 years ago

@vitorsr, @DanNixon, and @lucaferranti - 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. @editorialbot commands need to be the first thing in a new comment.

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, reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#4722 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 either of you require some more time. We can also use editorialbot (our bot) to set automatic reminders if you know you'll be away for a known period of time.

Please feel free to ping me (@danielskatz) if you have any questions/concerns.

vitorsr commented 2 years ago

Review checklist for @vitorsr

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

DanNixon commented 2 years ago

Review checklist for @DanNixon

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

lucaferranti commented 2 years ago

Review checklist for @lucaferranti

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

danielskatz commented 2 years ago

πŸ‘‹ @vitorsr, @DanNixon, and @lucaferranti - It's great to see you all have started your reviews. However, I don't see any open issues, so I wonder if you can let me know where you are, and anything that might be blocking you from making progress that either I or the authors can act on. Thanks!

lucaferranti commented 2 years ago

Apologies for the delay. This whole week I have been travelling for work. I'll carry the review during the upcoming week

danielskatz commented 2 years ago

πŸ‘‹ @vitorsr, @DanNixon, and @lucaferranti - Can let me know where you are, and anything that might be blocking you from making progress that either I or the authors can act on? Thanks!

DanNixon commented 2 years ago

Sorry, have not had a chance to look at this over the past week, should be able to get the rest of my review done this week.

danielskatz commented 2 years ago

πŸ‘‹ @DanNixon - any update?

danielskatz commented 2 years ago

πŸ‘‹ @lucaferranti - any update?

danielskatz commented 2 years ago

πŸ‘‹ @vitorsr - how is your review going?

lucaferranti commented 2 years ago

Apologies for the huge delay, pretty busy period. I have now allocated Saturday 8th morning (European time) to this.

Small side-comment: when I previously reviewed to JOSS, I was put as assignee on the review issue. This was very helpful, because I generally start and end my workday by checking what is assigned to me on github and so I did not forget about it (it also made finding the issue easier). Has something changed? Personally, I think it would help reviewers to have them as assignees in the issue.

vitorsr commented 2 years ago

Hi @danielskatz, this is a somewhat large project and I am carefully reviewing it piecewise as time allows. Please let me know if there is any urgency.

On Wed, Oct 5, 2022 at 4:53 AM Luca Ferranti @.***> wrote:

Apologies for the huge delay, pretty busy period. I have now allocated Saturday 8th morning (European time) to this.

Small side-comment: when I previously reviewed to JOSS, I was put as assignee on the review issue. This was very helpful, because I generally start and end my workday by checking what is assigned to me on github and so I did not forget about it (it also made finding the issue easier). Has something changed? Personally, I think it would be helpful

β€” Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/4722#issuecomment-1268078802, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJW2VYPZ33QLTA5YIUZPKDLWBUXYTANCNFSM6AAAAAAQBW2OXM . You are receiving this because you were mentioned.Message ID: @.***>

danielskatz commented 2 years ago

Hi @danielskatz, this is a somewhat large project and I am carefully reviewing it piecewise as time allows. Please let me know if there is any urgency.

no, just checking to make sure progress is happening...

danielskatz commented 2 years ago

πŸ‘‹ @vitorsr - do you have any update on your status?

danielskatz commented 2 years ago

@DanNixon - do you have any update on your status?

danielskatz commented 2 years ago

πŸ‘‹ @lucaferranti - do you have any update on your status?

vitorsr commented 2 years ago

Hi @danielskatz, I will be concluding my review shortly. The project is spotless. Any and all comments that I would have are mainly curiosities with reference to the subject itself in my capacity as a signal processing researcher, not as a reviewer with reference to the project code submitted to JOSS.

On Thu, Oct 20, 2022 at 2:03 PM Daniel S. Katz @.***> wrote:

πŸ‘‹ @vitorsr https://github.com/vitorsr - do you have any update on your status?

β€” Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/4722#issuecomment-1285879871, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJW2VYP6PJNKGXV4SGXFSHDWEF3MVANCNFSM6AAAAAAQBW2OXM . You are receiving this because you were mentioned.Message ID: @.***>

vitorsr commented 2 years ago

A small note on my review. I could not easily provision a GPU and I have had to do it on my personal computer, so my peruse has been bound to CPU-only testing (as the authors do in their CI).

On Thu, Oct 20, 2022 at 2:03 PM Daniel S. Katz @.***> wrote:

πŸ‘‹ @vitorsr https://github.com/vitorsr - do you have any update on your status?

β€” Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/4722#issuecomment-1285879871, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJW2VYP6PJNKGXV4SGXFSHDWEF3MVANCNFSM6AAAAAAQBW2OXM . You are receiving this because you were mentioned.Message ID: @.***>

lucaferranti commented 2 years ago

@editorialbot generate pfg

editorialbot commented 2 years ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@editorialbot commands

lucaferranti commented 2 years ago

@editorialbot generate pdf

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

lucaferranti commented 2 years ago

Dear @danielskatz and @bwohlberg πŸ‘‹ ,

First, I would like to start by sincerely apologizing for the huuuge delay in carrying my review. I have been a lot of deadlines and being the software pretty big it took a while to go through the code and examples. Again, I am very sorry for the delay, but finally I got there.

So, in general I would say the software and the paper have an exceptionally high quality and I definitely recommend for publication.

The software is very well documented and I particularly like how it supports both binder and colab to allow users to start playing with the examples. I have also run the examples locally and they seem to work appropriately.

The paper is in general well written. I only have one comment:

lucaferranti commented 2 years ago

@editorialbot check references

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

OK DOIs

- 10.1137/080716542 is OK
- 10.1137/1.9781611974997 is OK
- 10.1561/2200000016 is OK
- 10.1007/s10851-010-0251-1 is OK
- 10.1117/12.766355 is OK
- 10.1002/cpa.20042 is OK
- 10.1137/09076934x is OK
- 10.1016/0898-1221(76)90003-1 is OK
- 10.1109/TIP.2017.2713099 is OK
- 10.1109/LSP.2017.2763583 is OK
- 10.1109/MSP.2020.3016905 is OK
- 10.1007/978-0-387-40065-5 is OK
- 10.1561/2400000003 is OK
- 10.1561/2400000003 is OK
- 10.1109/iccv.2011.6126441 is OK
- 10.1109/TCI.2016.2599778 is OK
- 10.1109/GlobalSIP.2013.6737048 is OK
- 10.1090/s0025-5718-2012-02598-1 is OK
- 10.1109/TIP.2017.2662206 is OK

MISSING DOIs

- None

INVALID DOIs

- None
lucaferranti commented 2 years ago

Regarding the references, the list of DOIs above seems quite longer than the list of references in the paper, indeed the references.bib seem to have several references which are not cited in the paper. Could you clarify on their role?

@danielskatz I presume the references file should only contain the references which are actually used in the paper? Can you comment from an editor perspective on this?

danielskatz commented 2 years ago

@lucaferranti - regarding the references, our reference checker checks everything in the bib file, so references that are not used in the paper don't need to be there, but as long as they aren't causing problems in the check, they won't cause any problems anywhere else either

danielskatz commented 2 years ago

Dear @danielskatz and @bwohlberg πŸ‘‹ ,

First, I would like to start by sincerely apologizing for the huuuge delay in carrying my review. I have been a lot of deadlines and being the software pretty big it took a while to go through the code and examples. Again, I am very sorry for the delay, but finally I got there.

So, in general I would say the software and the paper have an exceptionally high quality and I definitely recommend for publication.

The software is very well documented and I particularly like how it supports both binder and colab to allow users to start playing with the examples. I have also run the examples locally and they seem to work appropriately.

The paper is in general well written. I only have one comment:

  • [ ] currently there is no comparison with other software in the fields. The authors mention in the statement of need that there are other packages overlapping with SCICO functionalities and a few also support execution on GPU, but they don't mention by name any other library. I would encourage the authors to expand and cite relevant software in the field; ideally also comparing SCICO to them in more details, e.g. in table form.

Thanks for these comments. @bwohlberg, it looks like there is just one thing here for you to respond to

bwohlberg commented 2 years ago

Thank you @lucaferranti and @danielskatz. The requested comparison with other software can be found in section Related Packages on this docs page (I suspect that @lucaferranti did not notice it because it was added relatively recently). We have not intentionally included any references that are not cited at least once (some may only be cited in a single example script), but it is possible that such references have inadvertently been created during changes to the docs.

DanNixon commented 1 year ago

Hi all, apologies from me also that my review has taken longer than I said it would, a few things came up that meant I only had the odd bit of time in the evenings to look at the software itself.

Anyway, I am happy with what I have seen and don't have any significant comments beyond what has already been said. The project is very well put together with good documentation and extensive examples/use cases.

danielskatz commented 1 year ago

@DanNixon - you have an item in your checklist that is unchecked. Can you check it off, or let us know what you would need done in order to do so?

danielskatz commented 1 year ago

@vitorsr - you have a few items left in your checklist unchecked. Can you check them off, or let us know what you would need in order to do so (which could include more time on your part, or actions from the author)?

vitorsr commented 1 year ago

All done. My only concern is that the JAX dependency seems a bit fragile - is there any plan to "future-proof" this requirement?

bwohlberg commented 1 year ago

All done. My only concern is that the JAX dependency seems a bit fragile - is there any plan to "future-proof" this requirement?

We have experienced some issues with changes in jaxlib and jax breaking parts of our code, but a recent re-write of the BlockArray implementation has made that much less frequent. The project requirements.txt file includes both minimum and maximum jaxlib / jax version numbers to ensure that the user installs versions of these dependencies that are known to function correctly with scico, and we've set up a weekly job in the project CI to check whether there are any compatibility issues with any new jaxlib / jax releases. We're open to suggestions, though, if there's anything else that could be done that we've overlooked.

danielskatz commented 1 year ago

πŸ‘‹ @vitorsr, @DanNixon, @lucaferranti - you've all now checked off all your review items, which I presume means that you think this can be accepted. Please confirm this is the case.

vitorsr commented 1 year ago

Yes. The submission has no acceptance blockers and meets all acceptance criteria.

On Tue, Oct 25, 2022 at 10:35 AM Daniel S. Katz @.***> wrote:

πŸ‘‹ @vitorsr https://github.com/vitorsr, @DanNixon https://github.com/DanNixon, @lucaferranti https://github.com/lucaferranti - you've all now checked off all your review items, which I presume means that you think this can be accepted. Please confirm this is the case.

β€” Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/4722#issuecomment-1290569779, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJW2VYIAS5VTCY5AXP5TYDDWE7O2TANCNFSM6AAAAAAQBW2OXM . You are receiving this because you were mentioned.Message ID: @.***>

DanNixon commented 1 year ago

Yes, also happy for the submission to be accepted.

lucaferranti commented 1 year ago

confirm the submission is ready for publication

danielskatz commented 1 year ago

great, thanks all.

danielskatz commented 1 year ago

πŸ‘‹ @bwohlberg - at this point could you:

I can then move forward with the next steps, which will include me proofreading the paper, perhaps suggesting changes, then accepting and publishing the submission.

bwohlberg commented 1 year ago

Thank you @vitorsr, @DanNixon, and @lucaferranti for your reviews.

@danielskatz: We released version 0.0.3 about a month ago (well after the review had started) and have only made minor changes since then. Would it be OK to use that tagged release for the process outlined above? Also, any objections to some minor changes (e.g. correcting a reference, updating a description of the repo) being made to the paper itself at this stage?

danielskatz commented 1 year ago

Both of those are fine.

bwohlberg commented 1 year ago

The tag is v0.0.3 and the Zenodo doi is 10.5281/zenodo.7255839.

bwohlberg commented 1 year ago

@editorialbot generate pdf