openjournals / joss-reviews

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

[REVIEW]: GCM-Filters: A Python Package for Diffusion-based Spatial Filtering of Gridded Data #3947

Closed whedon closed 2 years ago

whedon commented 2 years ago

Submitting author: @NoraLoose (Nora Loose) Repository: https://github.com/ocean-eddy-cpt/gcm-filters Version: v0.2.3 Editor: @elbeejay Reviewer: @callumrollo, @AleksiNummelin, @isgiddy Archive: 10.5281/zenodo.6039860

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

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

@callumrollo & @AleksiNummelin & @isgiddy, 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 @elbeejay 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 @callumrollo

✨ 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 @AleksiNummelin

✨ 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 @isgiddy

✨ 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. @callumrollo, @AleksiNummelin, @isgiddy 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 1131

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

OK DOIs

- 10.1029/2021MS002552 is OK
- 10.5334/jors.148 is OK
- 10.25080/majora-7b98e3ed-013 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1029/2019MS001726 is OK
- 1721.1/117188 is OK
- 10.5281/zenodo.4968496 is OK

MISSING DOIs

- 10.25080/majora-7b98e3ed-013 may be a valid DOI for title:  Dask: Parallel Computation with Blocked algorithms and Task Scheduling 

INVALID DOIs

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

github.com/AlDanial/cloc v 1.88  T=0.15 s (233.5 files/s, 178551.3 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          10            428            366           1502
Jupyter Notebook                 8              0          23079            879
reStructuredText                 6            219            182            259
TeX                              1             20              0            204
YAML                             7             12             15            184
Markdown                         2             35              0            114
TOML                             1              2              0             11
make                             1              4              7              9
-------------------------------------------------------------------------------
SUM:                            36            720          23649           3162
-------------------------------------------------------------------------------

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

Author                     Commits    Insertions      Deletions    % of changes
Andrew Ross                      8            89             20            1.10
Arthur                          24           483            420            9.10
Elizabeth A Yankovsk             6           132             41            1.74
Gustavo Marques                 10           632            198            8.37
Ian Grooms                      30           456            305            7.67
Julius Busecke                   6           104            209            3.16
NoraLoose                      116          3111           1557           47.06
Ryan Abernathey                 28          1461            686           21.64
Scott Bachman                    2             8              8            0.16

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
Andrew Ross                  70           78.7          0.9               14.29
Arthur                        5            1.0          7.9                0.00
Elizabeth A Yankovsk         25           18.9          1.0               16.00
Gustavo Marques             249           39.4          5.6               10.84
Ian Grooms                  201           44.1          5.7                5.97
Julius Busecke               83           79.8          9.2               39.76
NoraLoose                   936           30.1          4.6               10.36
Ryan Abernathey             727           49.8          5.9                8.12
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:

elbeejay commented 2 years ago

@callumrollo, @AleksiNummelin, and @isgiddy, thanks for agreeing to review this submission to JOSS. We are currently asking reviewers to try and complete their reviews in 6 weeks.

The JOSS review process is entirely open and transparent, and takes place on GitHub. Review comments can be made as issues in the GCM-Filters repository, please link this review issue when doing so (paste https://github.com/openjournals/joss-reviews/issues/3947 into the issue).

For reference, here are links to the JOSS documentation that may be helpful as you conduct your reviews:

Please feel free to ping me (@elbeejay) if you have any questions/concerns. Thanks again for agreeing to review for JOSS.

NoraLoose commented 2 years ago

@whedon generate pdf

I have added one sentence to the acknowledgments of the paper - hope that was okay!

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:

callumrollo commented 2 years ago

Hi Nora, this looks like a solid submission for JOSS. I'm not a modeller, but will review the code as best I can. I have a question about authorship. Most of the authors have made contributions to the codebase, corresponding to the author order. However, I could not find code contributions by Laure Zanna. Could you please outline their contribution to GCM_Filters?

NoraLoose commented 2 years ago

Hi @callumrollo, thanks so much for reviewing our submission!

We defined the authorship criteria here, following the JOSS recommendations.

The GCM-Filters package is being developed by the Ocean Eddy Climate Process Team (CPT). (The CPT is also the github organization that owns the GCM-Filters repo.) Laure Zanna does not only lead the CPT, but has also made contributions through discussing code & package ideas and the implementation of the MOM5 Laplacians (partially off github's eye, in our CPT meetings as well as in her research group meetings with Elizabeth Yankovsky and Arthur Guillaumin). She is invited as a co-author because her contributions clearly satisfy the following JOSS criterion:

Purely financial (such as being named on an award) and organizational (such as general supervision of a research group) contributions are not considered sufficient for co-authorship of JOSS submissions, but active project direction and other forms of non-code contributions are.

I hope this explanation helps! I'm happy to answer any further questions and concerns.

whedon commented 2 years ago

:wave: @AleksiNummelin, please update us on how your review is going (this is an automated reminder).

whedon commented 2 years ago

:wave: @isgiddy, please update us on how your review is going (this is an automated reminder).

whedon commented 2 years ago

:wave: @callumrollo, please update us on how your review is going (this is an automated reminder).

callumrollo commented 2 years ago

There is a notebooks folder in the top level of the repo, with a jupyter notebook enticingly named tutorial.ipynb. However, these appears to be a demo for a lesson on packing or similar? Not sure what it's doing here. I'd suggest removing it lest potential users mistake it for a demo of the package functionality.

The documentation contains some great examples of package functionality. Perhaps these could be dumped into a notebook for ease of experimentation by potential users?

callumrollo commented 2 years ago

Reading the API documentation, I am unclear on the difference between these two methods of the Filter class:

apply(self, field_or_dataset, dims): """Filter a field or xarray dataset with scalar Laplacian across the dimensions specified by dims."""

apply_to_field(self, field, dims): """Filter a field with scalar Laplacian across the dimensions specified by dims."""

If apply can process a field or dataset, apply_to_field seems superfluous. If they are performing different functions, this could be better explained in their docstrings

callumrollo commented 2 years ago

While not essential, the maintainers might consider copying the contributing information on the docs website to CONTRIBUTING.md in the repo so github detects it and users can find it more easily

callumrollo commented 2 years ago

Similarly Issue and PR templates could make it easier for people to submit bug reports/requests etc. Again not an essential recommendation, it looks like the project is already dealing well with Issues and PRs

callumrollo commented 2 years ago

Overall a very impressive package. Some of the workings are a bit beyond me, but I managed to run some of the example code and mess around with filter kernels. Great testing suite! Once above small issues are considered, I'm happy for this to go to publication

NoraLoose commented 2 years ago

Thanks for your comments and suggestions, @callumrollo! The 2 PRs and 2 issues that I linked above are in response to your 4 comments. We will try to resolve these issues and PRs asap.

isgiddy commented 2 years ago

I've just started working my way through the documentation. It's very explicit and I've managed to work through all the examples and haven't run into any issues so far. The package is user friendly and a great contribution.

Some minor suggestions so far:

I'll aim to finish up going through everything this week

elbeejay commented 2 years ago

Hi all, glad to see things are moving in a positive direction here.

A quick note to our reviewers @callumrollo, @isgiddy, and @AleksiNummelin; if you could make your comments and suggestions related to the review as issues in the GCM-Filters repository itself that is our preference. This keeps the review comments within the project repository should someone go back through the commit history and PRs and look for why any particular changes were made. This can be done either by making individual issues for each comment, or by putting all of your comments related to the review into a single issue.

In the issue, please remember to state that the issue is related to your JOSS review, and include the URL for this issue (https://github.com/ocean-eddy-cpt/gcm-filters), so that it is connected to this review issue.

isgiddy commented 2 years ago

I've tested a few of the filter kernels now on an independent dataset. I'm happy for this to go to forward to publication.

AleksiNummelin commented 2 years ago

Hi @elbeejay, sorry for being slow, I've started to look through the submission, but I had a simple question. I think I can't check the checkboxes above (should I be able to check them in the browser?), and I realized that I may not have accepted the invite and therefore I don't appear as an assignee, nor as a participant? Would it be possible to renew the invite, or is there something else that I am missing? @NoraLoose I will create an issue to the project repo with some comments.

elbeejay commented 2 years ago

Hi @elbeejay, sorry for being slow, I've started to look through the submission, but I had a simple question. I think I can't check the checkboxes above (should I be able to check them in the browser?), and I realized that I may not have accepted the invite and therefore I don't appear as an assignee, nor as a participant? Would it be possible to renew the invite, or is there something else that I am missing?

No problem, I will re-issue the review invitation.

elbeejay commented 2 years ago

@whedon re-invite @AleksiNummelin as reviewer

whedon commented 2 years ago

OK, the reviewer has been re-invited.

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

AleksiNummelin commented 2 years ago

Okay, I've finally gone through the review and submitted an issue related to this at https://github.com/ocean-eddy-cpt/gcm-filters/issues/124

I think it is fine to publish the paper before that issue is fully resolved since those are rather minor points, although @NoraLoose might want to give a quick try solving the issue with filtering on a tripolar grid.

NoraLoose commented 2 years ago

Hi all!

Thanks for your comments and identifying issues - GCM-Filters is already seeing a lot of improvement thanks to your reviews! Most of our developers have already unplugged for the winter break, so I would like to resolve the outstanding issues and PRs in the first week of January.

I'm tracking the issues that you have raised and that still need to be resolved here. (Everyone feel free to link more issues!)

Happy holidays, everyone!

callumrollo commented 2 years ago

Thanks Nora, great effort! I'm off on a cruise in a couple of weeks. I won't be contactable, but I'm happy for this to be approved for publication once the linked issues are resolved.

elbeejay commented 2 years ago

Thanks for your comments and identifying issues - GCM-Filters is already seeing a lot of improvement thanks to your reviews! Most of our developers have already unplugged for the winter break, so I would like to resolve the outstanding issues and PRs in the first week of January.

@NoraLoose this sounds great, we will plan to revisit the submission in January in this case. Thanks @isgiddy, @AleksiNummelin, and @callumrollo, it seems like this review process has been reasonably smooth.

AleksiNummelin commented 2 years ago

Dear @elbeejay, @NoraLoose and the team have made a really nice effort and answered my questions at https://github.com/ocean-eddy-cpt/gcm-filters/issues/124 so I consider that issue closed (left that for the authors), which means that this review is also complete from my side - happy to see this published.

elbeejay commented 2 years ago

Cool, thanks @AleksiNummelin! @NoraLoose, let me know when you and your co-authors have made all of the revisions you'd like to make prior to JOSS publication. I noticed you created a "project" to track the issues which is neat. There's no timeline or deadline that you need to stick to at this point as all 3 reviewers have signed off, so I leave it up to you. Just ping me here when you'd like to move on to the final steps of the process.

NoraLoose commented 2 years ago

Thanks @elbeejay! We have finished the last revisions (all tasks in the JOSS project are moved to "Done"). So I think we are ready to move on to the final steps.

Before doing so, I would like to make a small request. Between when we started the review and now, Paige Martin (@paigem) has started to work on PR https://github.com/ocean-eddy-cpt/gcm-filters/pull/128. The PR is not merged yet, but Paige has already made great progress toward an awesome new feature for our package! Therefore I have invited Paige as a co-author onto the JOSS paper. Sorry for the last-minute change, but I hope everyone here agrees with having Paige added to the author list.

Thanks again for your reviews and your super helpful feedback, @callumrollo, @isgiddy, and @AleksiNummelin!

NoraLoose commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

PDF failed to compile for issue #3947 with the following error:

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

elbeejay 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.1029/2021MS002552 is OK
- 10.5334/jors.148 is OK
- 10.25080/majora-7b98e3ed-013 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1029/2019MS001726 is OK
- 1721.1/117188 is OK
- 10.5281/zenodo.4968496 is OK

MISSING DOIs

- 10.25080/majora-7b98e3ed-013 may be a valid DOI for title:  Dask: Parallel Computation with Blocked algorithms and Task Scheduling 

INVALID DOIs

- None
elbeejay commented 2 years ago

@NoraLoose that all sounds good :+1:

I'm going to give this a closer look over the next few days just to make sure there are no typos and the like in the paper and I will get back to you with next steps shortly.

elbeejay commented 2 years ago

Hi @NoraLoose, I think the paper reads well. My only minor suggestions are to consider rewording/rephrasing in a few sections (see below) where ndimage.gaussian_filter and GCM-Filters end up being broken across lines. Up to you, but could be more aesthetically pleasing.

image

image

Whether or not you reword any parts of the paper, I'm happy for us to continue along the JOSS publication process. So at this time I'd like to ask if you could:

Doing so will enable us to square away the necessary metadata before this is published.

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

NoraLoose commented 2 years ago

Thanks @elbeejay for the catching the line breaks in the paper!

I just did some minor re-wording to avoid those. I also bumped up the version to 0.2 (so far only in the paper, will do the tagged release next).

This PR contains the exact changes to the paper.

NoraLoose commented 2 years ago
NoraLoose commented 2 years ago

@whedon generate pdf

To be consistent between zenodo archive and JOSS paper, I changed v0.2 to v0.2.3 in the paper.

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:

elbeejay commented 2 years ago

@whedon set v0.2.3 as version

whedon commented 2 years ago

OK. v0.2.3 is the version.

elbeejay commented 2 years ago

@whedon set 10.5281/zenodo.6039860 as archive