search

openjournals / joss-reviews

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

[REVIEW]: regional-mom6: A Python package for automatic generation of regional configurations for the Modular Ocean Model 6 #6857

Open editorialbot opened 1 month ago

editorialbot commented 1 month ago

Submitting author: !--author-handle-->@ashjbarnes<!--end-author-handle-- (Ashley Barnes) Repository: https://github.com/COSIMA/regional-mom6/ Branch with paper.md (empty if default branch): joss-paper Version: v0.5.0 Editor: !--editor-->@AnjaliSandip<!--end-editor-- Reviewers: @alperaltuntas, @dionhaefner, @theresa-morrison Archive: Pending

Status

status

Status badge code:

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

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

@alperaltuntas & @dionhaefner & @theresa-morrison, 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 @AnjaliSandip 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 @alperaltuntas

📝 Checklist for @dionhaefner

📝 Checklist for @theresa-morrison

editorialbot commented 1 month 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 month ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.03 s (900.0 files/s, 186761.1 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                           7            385            567           1936
Markdown                         6            138              0            372
TeX                              1             29              0            272
Jupyter Notebook                 2              0           1736            195
YAML                             8             26             19            179
reStructuredText                 3             30             22             56
TOML                             1              5              0             31
make                             1              4              7              9
-------------------------------------------------------------------------------
SUM:                            29            617           2351           3050
-------------------------------------------------------------------------------

Commit count by author:

   370  Navid C. Constantinou
   137  ashjbarnes
   106  navidcy
    33  Ashley Barnes
    25  Angus Gibson
     4  Andrew Kiss
     3  Dhruv Bhagtani
     1  Chris Chapman
     1  John Reilly
     1  dhruvbhagtani
     1  reillyja
editorialbot commented 1 month ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1029/2019MS001726 is OK
- 10.1029/96JC02775 is OK
- 10.1016/j.ocemod.2004.08.002 is OK
- 10.5281/zenodo.4294774 is OK
- 10.5285/f98b053b-0cbc-6c23-e053-6c86abc0af7b is OK
- 10.48670/moi-00021 is OK
- 10.5194/gmd-16-1481-2023 is OK
- 10.48670/moi-00021 is OK
- 10.5194/egusphere-2024-394 is OK
- 10.5194/gmd-16-6943-2023 is OK
- 10.1016/B978-0-12-460817-7.50009-4 is OK
- 10.1016/j.ocemod.2010.12.008 is OK
- 10.1029/2019MS001954 is OK
- 10.48550/arXiv.2309.06662 is OK
- 10.1016/B978-0-12-821512-8.00015-3 is OK
- 10.1016/B978-0-12-821512-8.00010-4 is OK
- 10.1016/B978-0-12-821512-8.00009-8 is OK
- 10.1016/0021-9991(76)90023-1 is OK
- 10.1017/CBO9780511618291 is OK
- 10.1016/S1463-5003(01)00012-9 is OK
- 10.5194/gmd-16-1481-2023 is OK
- 10.5194/gmd-13-401-2020 is OK
- 10.1016/j.ocemod.2018.07.002 is OK

MISSING DOIs

- No DOI given, and none found for title: tasman-tides
- No DOI given, and none found for title: MITgcm_python
- No DOI given, and none found for title: Pyroms
- No DOI given, and none found for title: Earth System Modeling Group (ESMG) gridtools
- No DOI given, and none found for title: Elements of the modular ocean model (MOM)
- No DOI given, and none found for title: CEFI-regional-MOM6: Essential tools, XML files, an...

INVALID DOIs

- None
editorialbot commented 1 month ago

Paper file info:

📄 Wordcount for paper.md is 1676

✅ The paper includes a Statement of need section

editorialbot commented 1 month ago

License info:

✅ License found: MIT License (Valid open source OSI approved license)

editorialbot commented 1 month ago

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

dionhaefner commented 1 month ago

Review checklist for @dionhaefner

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

AnjaliSandip commented 1 month ago

@alperaltuntas, and @theresa-morrison here's a friendly reminder to get started with the review process

alperaltuntas commented 2 weeks ago

Review checklist for @alperaltuntas

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

AnjaliSandip commented 2 weeks ago

@dionhaefner How close are you to completing the review?

AnjaliSandip commented 2 weeks ago

@theresa-morrison, here's a friendly reminder to get started with the review process

dionhaefner commented 2 weeks ago

I couldn't test the software myself since I don't have a MOM6 setup, but looking at the tests and CI setup gave me enough confidence to conclude that the project is in good working order. Perhaps the authors want to consider adding integration via binder to allow for easier experimentation with the package.

Based on that I have no notes; looks like a really useful package with sound software engineering and UX. I'm not the biggest fan of requiring conda but I can see how that may be the path of least resistance for the target audience. The need and capabilities of the software are well described in both docs and paper. I congratulate the authors on creating a great piece of scientific software.

AnjaliSandip commented 2 weeks ago

Thanks @dionhaefner. In your opinion, is the submission acceptable for publication?

dionhaefner commented 2 weeks ago

yes

theresa-morrison commented 2 weeks ago

Review checklist for @theresa-morrison

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

alperaltuntas commented 2 weeks ago

I have reviewed all the items on my checklist and confirm that the submission is acceptable for publication. My only suggestion for the authors is to consider eliminating the dependency on FRE-NCtools. This dependency complicates the build process unnecessarily. The authors use only three tools from FRE-NCtools, and I believe these functionalities can be implemented in Python within the regional-mom6 package with little to moderate effort. This change would result in a more complete package and a simpler installation process.

ashjbarnes commented 2 weeks ago

I have reviewed all the items on my checklist and confirm that the submission is acceptable for publication. My only suggestion for the authors is to consider eliminating the dependency on FRE-NCtools. This dependency complicates the build process unnecessarily. The authors use only three tools from FRE-NCtools, and I believe these functionalities can be implemented in Python within the regional-mom6 package with little to moderate effort. This change would result in a more complete package and a simpler installation process.

Thanks @alperaltuntas I agree that this would be good to do. I believe there are some functions that could help us in the ESMG gridtools repo. We've been meaning for a while to include this repo as a dependency (and acknowledging appropriately) and wrap some of the python grid generation functions to expand the functionality of regional-mom6. Extra dependencies might also complicate building and maintainability though of course! What are your thoughts on this?

theresa-morrison commented 2 weeks ago

I have reviewed all the items on my checklist and confirm the package is acceptable for publication. I tested the package using the demo and was able to and it worked as promised. I am excited to have these tools available to use!

navidcy commented 2 weeks ago

My only suggestion for the authors is to consider eliminating the dependency on FRE-NCtools. This dependency complicates the build process unnecessarily. The authors use only three tools from FRE-NCtools, and I believe these functionalities can be implemented in Python within the regional-mom6 package with little to moderate effort. This change would result in a more complete package and a simpler installation process.

Thanks for your comments @alperaltuntas!

@ashjbarnes should we make 3 issues in http://github.com/COSIMA/regional-mom6/issues for these 3 functionalities that @alperaltuntas is referring to? Or if we have opened issues already, could we link them to this review?

ashjbarnes commented 2 weeks ago

This issue is about incorporating the Grid Gen tools. I'll update the description to mention the replacement of FRE-NCtools. I think these two things would have to be done together in one PR, but up to you if we split into separate issues.

navidcy commented 2 weeks ago

thanks @theresa-morrison, @dionhaefner, @alperaltuntas, for your comments and efforts in reviewing! I thought I was watching this issue but I wasn't so I wasn't getting any notifications... I just saw all the posts here. Thanks!

AnjaliSandip commented 1 week ago

@ashjbarnes, Could you address the comments by @alperaltuntas?

alperaltuntas commented 1 week ago

@AnjaliSandip my comments were only suggestions for future work and should not delay the publication of the paper, I hope.

ashjbarnes commented 1 week ago

I have reviewed all the items on my checklist and confirm that the submission is acceptable for publication. My only suggestion for the authors is to consider eliminating the dependency on FRE-NCtools. This dependency complicates the build process unnecessarily. The authors use only three tools from FRE-NCtools, and I believe these functionalities can be implemented in Python within the regional-mom6 package with little to moderate effort. This change would result in a more complete package and a simpler installation process.

Thanks @alperaltuntas I agree that this would be good to do. I believe there are some functions that could help us in the ESMG gridtools repo. We've been meaning for a while to include this repo as a dependency (and acknowledging appropriately) and wrap some of the python grid generation functions to expand the functionality of regional-mom6. Extra dependencies might also complicate building and maintainability though of course! What are your thoughts on this?

@AnjaliSandip Please see my response here. I'd be interested in feedback as to whether we should add this existing Python library as a dependency, or write our own FRE-NCtools in Python.

AnjaliSandip commented 1 week ago

@alperaltuntas In your opinion, is the submission acceptable for publication?

ashjbarnes commented 4 days ago

I have reviewed all the items on my checklist and confirm that the submission is acceptable for publication. My only suggestion for the authors is to consider eliminating the dependency on FRE-NCtools. This dependency complicates the build process unnecessarily. The authors use only three tools from FRE-NCtools, and I believe these functionalities can be implemented in Python within the regional-mom6 package with little to moderate effort. This change would result in a more complete package and a simpler installation process.

@AnjaliSandip I think @alperaltuntas has already indicated this in their reply above

alperaltuntas commented 3 days ago

@alperaltuntas In your opinion, is the submission acceptable for publication?

@AnjaliSandip Yes, I approve.

I'd be interested in feedback as to whether we should add this existing Python library as a dependency, or write our own FRE-NCtools in Python.

@ashjbarnes I am unfamiliar with gridtools, but sounds like a good alternative.