editorialbot commented 1 year ago

Submitting author: !--author-handle-->@alextbradley@observingClouds<!--end-editor-- Reviewers: @PennyHow, @daniel-cheng, @JordiBolibar Archive: 10.5281/zenodo.10723504

Status

Status badge code:

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

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

@PennyHow & @daniel-cheng, 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 @observingClouds 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 @PennyHow

📝 Checklist for @daniel-cheng

📝 Checklist for @JordiBolibar

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.10 s (1513.4 files/s, 288705.1 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
CSS                              2            701             49          14825
Julia                           59            967            477           4625
Markdown                        36            547              0           2224
SVG                              4              0              0           2078
JavaScript                       5             44             78            511
HTML                            32             25              0            501
TeX                              2             35              0            375
Lisp                             3             24              0            243
YAML                             2              3              4             41
TOML                             2              1              0             36
Dockerfile                       1              3              0              4
JSON                             1              0              0              4
-------------------------------------------------------------------------------
SUM:                           149           2350            608          25467
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository

editorialbot commented 1 year ago

Wordcount for paper.md is 947

editorialbot commented 1 year ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1038/s41586-021-03302-y is OK
- 10.1038/nclimate2094 is OK
- 10.1038/s41586-021-03427-0 is OK
- 10.5194/tc-15-113-2021 is OK
- 10.5281/zenodo.4318147 is OK
- 10.1126/sciadv.abg3080 is OK
- 10.1029/2006JF000664 is OK
- 10.1002/2014JF003239 is OK
- 10.1002/2017GL072514 is OK
- 10.3189/002214311795306763 is OK
- 10.1007/s40641-017-0071-0 is OK
- 10.1029/96JC02776 is OK
- 10.1016/j.jcp.2004.12.013 is OK
- 10.5194/tc-14-2283-2020 is OK
- 10.1029/2011JF002140 is OK
- 10.1029/2008JF001179 is OK
- 10.1016/j.jcp.2012.08.037 is OK
- 10.1088/1748-9326/aac2f0 is OK
- 10.1038/nature10968 is OK
- 10.1051/0004-6361/201629272 is OK
- 10.1051/0004-6361/201322068 is OK

MISSING DOIs

- 10.5194/gmd-6-1299-2013 may be a valid DOI for title: Capabilities and performance of Elmer/Ice, a new-generation ice sheet model

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:

observingClouds commented 1 year ago

@PennyHow, @daniel-cheng thank you very much for agreeing to review this submission. Please start your review by commenting here with @editorialbot generate my checklist to get an overview about the review criteria and your personal checklist. If you have any questions please mention me here.

I will just set up an automatic reminder in a week to see if there are any initial questions/issues.

observingClouds commented 1 year ago

@editorialbot remind @PennyHow in seven days

editorialbot commented 1 year ago

Reminder set for @PennyHow in seven days

observingClouds commented 1 year ago

@editorialbot remind @Daniel-cheng in seven days

editorialbot commented 1 year ago

Reminder set for @Daniel-cheng in seven days

observingClouds commented 1 year ago

@alextbradley as you can see the review process has started now 🎉

While the reviewers are busy reviewing your submission, could you please add the missing DOI of Gagliardini2013 to your paper.bib. Please note that @editorialbot's suggestion might not be correct. Afterwards you can run @editorialbot generate pdf to check if the MISSING DOI message is no longer appearing.

daniel-cheng commented 1 year ago

Review checklist for @daniel-cheng

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the https://github.com/RJArthern/WAVI.jl?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@alextbradley) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
[x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
[ ] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
[x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[ ] Functionality: Have the functional claims of the software been confirmed?
[ ] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[ ] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

PennyHow commented 1 year ago

Review checklist for @PennyHow

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the https://github.com/RJArthern/WAVI.jl?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@alextbradley) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
[x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
[x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
[x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

PennyHow commented 1 year ago

Hi @alextbradley et al. I've just had a look through your repo and the paper. Nice work! Overall I think this is a really valuable alternative to ice sheet models currently available. I don't think I have seen any openly available ice sheet models in Julia, so I think this is a first!

I have some queries about running the model and its documentation. From initially setting up Julia and downloading WAVI.jl through your documented set-up, I have some first comments.

Concerning the software

I am having problems running the MISMIP+ experiment outlined in your README here.

Should this line have some bracketed component?
```
bed = WAVI.mismip_plus_bed
```

And secondly, this line is throwing up this method error.


julia> model = Model(grid = grid,bed_elevation = bed, params = params)
ERROR: MethodError: no method matching WAVI.HGrid(::Int64, ::Int64, ::Matrix{Bool}, ::Int64, ::LinearAlgebra.Diagonal{Float64, Vector{Float64}}, ::Matrix{Float64}, ::SparseArrays.SparseMatrixCSC{Float64, Int64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Base.RefValue{LinearAlgebra.Diagonal{Float64, Vector{Float64}}}, ::Base.RefValue{LinearAlgebra.Diagonal{Float64, Vector{Float64}}})

Closest candidates are: WAVI.HGrid(::N, ::N, ::Matrix{Bool}, ::N, ::LinearAlgebra.Diagonal{T, Vector{T}}, ::SparseArrays.SparseMatrixCSC{T, N}, ::SparseArrays.SparseMatrixCSC{T, N}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Base.RefValue{LinearAlgebra.Diagonal{T, Vector{T}}}, ::Base.RefValue{LinearAlgebra.Diagonal{T, Vector{T}}}) where {T<:Real, N<:Integer} @ WAVI ~/.julia/packages/WAVI/osoCg/src/Fields/HGrid.jl:2

Stacktrace:e2 [1] WAVI.HGrid(; nxh::Int64, nyh::Int64, mask::Matrix{Bool}, b::Matrix{Float64}, h::Matrix{Float64}, ηav::Matrix{Float64}) @ WAVI ~/.julia/packages/WAVI/osoCg/src/Fields/HGrid.jl:144 [2] setup_fields(grid::Grid{Float64, Int64}, initial_conditions::InitialConditions{Float64}, solver_params::SolverParams{Float64, Int64}, params::Params{Float64, Matrix{Float64}, Matrix{Float64}, Matrix{Float64}}, bed_array::Matrix{Float64}) @ WAVI ~/.julia/packages/WAVI/osoCg/src/Fields/Fields.jl:41 [3] Model(; grid::Grid{Float64, Int64}, bed_elevation::Function, params::Params{Float64, Float64, Float64, Float64}, solver_params::SolverParams{Float64, Int64}, initial_conditions::InitialConditions{Float64}, melt_rate::UniformMeltRate{Float64}) @ WAVI ~/.julia/packages/WAVI/osoCg/src/Models/Model.jl:83 [4] top-level scope @ REPL[13]:1



I checked the other examples on your really nice documentation pages (https://rjarthern.github.io/WAVI.jl/). And I came across the same problem with this line in each of the examples. I'm not as familiar with Julia, so is this something I am doing wrong, something else I need in my set-up, or something that needs to be modified in your examples? Just FYI, I'm running Julia 1.9.0 on a Ubuntu 22.04 LTS machine.

**Concerning the documentation**

I had to search hard for your documentation at https://rjarthern.github.io/WAVI.jl/ as I don't think it is included in your repo's README. Could you add this as a link to the repo, or a badge (if this is possible with your `Documenter.jl `set-up)? It has some great theoretical summaries, examples and walkthroughs. 

With regard to the documentation, I have a request which I am happy to discuss with you the best way forward - Most packages use some form of autodocs to automatically generate documentation from the code (e.g. function arguments, outputs, and brief description). This would be highly beneficial to users to understand the capabilities and inputs to functions, troubleshoot problems, and adapt functions where needed. Could this be a possibility for you to include in your documentation?

**Concerning the software paper**

Great, easy-to-follow paper with a fantastic overview of current ice sheet modelling theory and available models. Just a couple of comments from me on this...

- L8: I would say "lower the bar" is quite a colloqial term. Please change this to something more like "`WAVI.jl` is designed to make ice sheet modelling more accessible to beginners and low-level users..."
- L25: What does the bracketed part of "On the long [O(1000s km)]..." mean"
- L25: "lengthscales" >> "length scales"

editorialbot commented 1 year ago

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

editorialbot commented 1 year ago

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

observingClouds commented 1 year ago

Hi @PennyHow, Hi @daniel-cheng, I see you are busy working through the checklist and have already made the first comments. It looks like we are all on track. Please be reminded to also make use of the issue functionality of GitHub and file issues on WAV.jl's repository. Discussions can quickly go out of hand on this thread here if there is a lot of back and forth.

@alextbradley, please feel free to already address some of the posted comments. If they require a longer response, I recommend opening issues on your end and tag the specific reviewer over there to continue the discussion.

Cheers!

observingClouds commented 1 year ago

@editorialbot remind @daniel-cheng in seven days

editorialbot commented 1 year ago

Reminder set for @daniel-cheng in seven days

observingClouds commented 1 year ago

@editorialbot remind @PennyHow in seven days

editorialbot commented 1 year ago

Reminder set for @PennyHow in seven days

observingClouds commented 1 year ago

@editorialbot remind @observingClouds in seven days

editorialbot commented 1 year ago

Reminder set for @observingClouds in seven days

observingClouds commented 1 year ago

@editorialbot add @JordiBolibar as reviewer

editorialbot commented 1 year ago

@JordiBolibar added to the reviewers list!

observingClouds commented 1 year ago

Hi everyone, Because JOSS generally tries to have three reviewers per submission, I'm delighted that @JordiBolibar excepted to review this submission as well and rounds up the expertise needed for a fair review of this submission. Thank you @JordiBolibar for signing up as a reviewer and accepting to review this submission. Please create your checklist by commenting below with @editorialbot generate my checklist and you should be ready to go.

Thank you!

observingClouds commented 1 year ago

@editorialbot remind @JordiBolibar in ten days

editorialbot commented 1 year ago

Reminder set for @JordiBolibar in ten days

JordiBolibar commented 1 year ago

Review checklist for @JordiBolibar

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the https://github.com/RJArthern/WAVI.jl?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@alextbradley) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
[x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
[x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
[x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

JordiBolibar commented 1 year ago

Review for WAVI.jl

Hi @alextbradley et al., I have finished reading your manuscript, repository and docs. Overall, great work and extremely happy to witness the first ice sheet model in Julia! I think the goal of the model is well presented, the API looks clean, and the docs are quite exhaustive. Nonetheless, I've come across some technical issues when trying to use the model, and I also have some feedback to potentially improve the docs.

Installation

During installation, following your guidelines, I encountered the following warning:

1 dependency had warnings during precompilation:
┌ WAVI [881191c6-26ca-4787-919d-8f98f1c41532]
│  WARNING: Method definition spdiagm(Integer, Integer, (Pair{var"#s87", var"#s86"} where var"#s86"<:(AbstractArray{T, 1} where T) where var"#s87"<:Integer)...) in module SparseArrays at /cache/build/default-amdci4-6/julialang/julia-release-1-dot-9/usr/share/julia/stdlib/v1.9/SparseArrays/src/sparsematrix.jl:3986 overwritten in module WAVI at /home/jovyan/.julia/packages/WAVI/osoCg/src/WAVI.jl:68.
│   ** incremental compilation may be fatally broken for this module **
└

There seems to be an issue with the package SparseArrays.jl, which as I will discuss in the next section, also hinders the use of your package.

Running the code

I have tried running some of your examples (both the one in the README and the simplest one in the docs), but the code seems to crash when initializing the grid:

grid = Grid(nx = 300, ny = 2, dx = 12000.0, dy = 12000.0)

which produces:

ERROR: MethodError: no method matching WAVI.HGrid(::Int64, ::Int64, ::Matrix{Bool}, ::Int64, ::LinearAlgebra.Diagonal{Float64, Vector{Float64}}, ::Matrix{Float64}, ::SparseArrays.SparseMatrixCSC{Float64, Int64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Matrix{Float64}, ::Base.RefValue{LinearAlgebra.Diagonal{Float64, Vector{Float64}}}, ::Base.RefValue{LinearAlgebra.Diagonal{Float64, Vector{Float64}}})

Closest candidates are:
  WAVI.HGrid(::N, ::N, ::Matrix{Bool}, ::N, ::LinearAlgebra.Diagonal{T, Vector{T}}, ::SparseArrays.SparseMatrixCSC{T, N}, ::SparseArrays.SparseMatrixCSC{T, N}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Matrix{T}, ::Base.RefValue{LinearAlgebra.Diagonal{T, Vector{T}}}, ::Base.RefValue{LinearAlgebra.Diagonal{T, Vector{T}}}) where {T<:Real, N<:Integer}
   @ WAVI ~/.julia/packages/WAVI/osoCg/src/Fields/HGrid.jl:2

I would say this is by far the most important issue to address in the review. Probably it's just a minor technical issue with the package, but it would be important to have everything running smoothly before widely sharing the library with the community.

Authors

I'm not familiar with the format of the authors in JOSS, but to me in the manuscript there seems to be 3 authors, and in the repository and the code there seems to be mainly two authors. Is this normal? Probably I'm misunderstanding something here, so apologies if I got this wrong.

Documentation

Access

I see this was also mentioned by @PennyHow, but I found it really hard to find the docs. Actually, I randomly came across them some weeks ago, but when I tried to look for the link in the repository during the review I just couldn't find it. I think a link and a mention of the docs should be clearly stated in the README (perhaps with a docs badge), and/or in the repository description.

API

The WAVI Setup section name is a little bit confusing to me. I feel it would be clearer to call it "API" or "WAVI data structures". Setup sounds more like configuration or installation to me.

The image showing the data structures hierarchy is not displaying in the docs. It would be nice to fix this, since I'm sure it's really useful to users.

The simulations section has some formatting issues, where some code is displayed as a title.

Physics

I find the term "melt rates" a bit vague. After reading the docs one can see that you are talking about parametrizations of ice shelf basal melt. Nonetheless, there are a lot of different physical processes involving melt on ice sheets (e.g. surface mass balance, basal mass blance, internal mass balance...). Perhaps being a little bit more specific would make things clearer for users.

References

This is quite irrelevant, but I find the format of the references to be a little bit crude.

Random

In the intro of the docs, it should read "at its heart" instead of "at it's heart".

I think it's very nice that you included so many different examples of increasing complexity. This really will help users to get started and understand the internals of the model. One little thing I'd fix there is the colorbar of the heatmaps. It's an annoying common issue in Julia, where you need to manually add an extra space in order to avoid an overlap between the title and the units.

README

You have a very nice logo, why not add it to the README? 🙂

Manuscript

Overall very nicely written and straight to the point. I really have almost no comments there, I think you managed to explain the field of ice sheet modelling to a wide audience, while providing enough details.

Comparison to the field

You show examples of other models, but you don’t really mention what are the main differences between them. It is clear that WAVI.jl is more user-friendly, but perhaps it would be interesting to know a little bit more on why it is different or similar to other models.

editorialbot commented 1 year ago

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

editorialbot commented 1 year ago

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

editorialbot commented 1 year ago

:wave: @observingClouds, please take a look at the state of the submission (this is an automated reminder).

editorialbot commented 1 year ago

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

observingClouds commented 1 year ago

Hi @JordiBolibar, @daniel-cheng, @PennyHow, You made great progress! Thank you very much for your in-depth review. If you could please tick off all the checkpoints in your checklist that you have tested and have provided feedback on (if necessary) that would be great. Any issues do not need to be resolved at this stage.

If you have finished your review by checking all checklist boxes, I will ask @alextbradley to address all comments.

Thank you.

PennyHow commented 1 year ago

You made great progress! Thank you very much for your in-depth review. If you could please tick off all the checkpoints in your checklist that you have tested and have provided feedback on (if necessary) that would be great. Any issues do not need to be resolved at this stage.

Hi @observingClouds, my checklist is as up-to-date as possible. Some points are not checked as the issues with the examples need to be addressed before I can continue with my review. I'm happy to circle back round once they have been resolved. Is that okay?

observingClouds commented 1 year ago

Hi @observingClouds, my checklist is as up-to-date as possible. Some points are not checked as the issues with the examples need to be addressed before I can continue with my review. I'm happy to circle back round once they have been resolved. Is that okay?

Yes, that works! Thanks for the update!

daniel-cheng commented 1 year ago

Hi @observingClouds, all, my checklist is up-to-date as well, but I am a bit delayed in completing it - I will be able to look over the manuscript next week though, and will should be able to evaluate the repository/code by the week after. I will be mindful of time though, and I'll be sure to update if I get things done sooner. Let me know if you need anything ahead of this schedule.

observingClouds commented 1 year ago

Hi everyone, It has been a while so I just want to check in with you. @daniel-cheng did you had a chance to finish your review? @alextbradley would you mind starting in the meantime with addressing the reviews, in particular the issue @PennyHow and @JordiBolibar are facing during the example execution so that they can finish their review? Thank you!

daniel-cheng commented 1 year ago

Hi @observingClouds, thanks for your patience on this - I've been able to review this submission and elaborate on the following items from the checklist:

General checks

The submission fulfills most checklist items, barring reproducibility of the results.
I encountered similar issues as my fellow reviewers in attempting to run the code, which I was not able to resolve on my own.

Functionality

Functionality and performance of the code are not currently verified barring resolution of the code issues mentioned above.

Documentation

There is a lack of clear contributing guidelines that should be added to the repository Read Me.
There are tests, but not a clear way to run them in the publicly available documentation (as mentioned by the other reviewers at https://rjarthern.github.io/WAVI.jl/)

Software paper

The manuscript is clear and concise, and shows how the development of this Julia based ice sheet flow model enhances accessibility and scientific study by providing tools for the cryospheric community.

observingClouds commented 1 year ago

Dear @daniel-cheng, Thank you very much for taking the time to review this submission and post your feedback. Cheers, Hauke

observingClouds commented 1 year ago

@alextbradley all reviews have been received now. Please go through the comments and issues and address them one-by-one here/or on your projects issue tracker. The reviewers indicated they had problems running some of the code and making their reviews incomplete. Please pay particular attention to this issue so that the reviewers can soon continue with the evaluation. Thank you!

alextbradley commented 1 year ago

Hi all! Thanks for the reviews, and apologies for being slow in responding - I have been on a sabbatical but now returning to work. I’ll get to these comments asap!

observingClouds commented 1 year ago

Thanks @alextbradley for the update. Please let us know when you have addressed the issues and we can continue the review.

alextbradley commented 1 year ago

Hi @observingClouds (cc @PennyHow, @JordiBolibar, @daniel-cheng), thanks again for convening the review process and to the reviewers for their reports so far. I just wanted to give a progress update: the SparseArrays issue which prevented the reviewers from running the example is taking us a little longer to fix than we had expected. (We had only tested our code on Julia versions < 1.6.2, and this issue is arising only with newer versions of Julia, which all reviewers are using -- sorry about that!) We're confident that this should be fixed soon, and I'll let you know as soon as!

alextbradley commented 11 months ago

Hi @observingClouds @PennyHow @JordiBolibar @daniel-cheng, pleased to report that the bug causing the problems is now fixed (commit: https://github.com/RJArthern/WAVI.jl/commit/65a27dfbc8e5d0a939099bafb1c21abf2ff5c30c) 🎉 🥳. We've started to adjust the code and repo in response to the comments above -- @observingClouds, should we respond to those comments now, or wait for the rest of the evaluation of the reviewers?

observingClouds commented 11 months ago

Hi @alextbradley, Thanks for the update. Please go ahead and respond to the reviewers' comments that were not affected by this bug so we can accelerate the review process. Cheers, Hauke

alextbradley commented 10 months ago

Hi @PennyHow @observingClouds @JordiBolibar @daniel-cheng :) Here is our response to the outstanding comments so far!

@PennyHow

Issues highlighted pre-review:

1) Should this [bed = WAVI.mismip_plus_bed] line have some bracketed component?

No, this generates the bed as a function, which can then be evaluated. In general, WAVI can accept numeric grids (i.e. where the bed is specified numerically at each grid point) or functions. The issue arising in the example was due to the bug highlighted in the next comment, which is now fixed.

2) I checked the other examples on your really nice documentation pages (https://rjarthern.github.io/WAVI.jl/). And I came across the same problem with this line in each of the examples. I'm not as familiar with Julia, so is this something I am doing wrong, something else I need in my set-up, or something that needs to be modified in your examples? Just FYI, I'm running Julia 1.9.0 on a Ubuntu 22.04 LTS machine.

Thanks for highlighting this issue. As outlined in the previous comment (https://github.com/openjournals/joss-reviews/issues/5584#issuecomment-1764952630), this was caused by incompatibility with the sparse arrays package in newer versions of julia, which were untested and should have now been fixed.

3) I had to search hard for your documentation at https://rjarthern.github.io/WAVI.jl/ as I don't think it is included in your repo's README. Could you add this as a link to the repo, or a badge (if this is possible with your Documenter.jl set-up)? It has some great theoretical summaries, examples and walkthroughs.

We've added a link in the readme and in the 'about' section of the repo, and sorry that it was hard to find!

4) With regard to the documentation, I have a request which I am happy to discuss with you the best way forward - Most packages use some form of autodocs to automatically generate documentation from the code (e.g. function arguments, outputs, and brief description). This would be highly beneficial to users to understand the capabilities and inputs to functions, troubleshoot problems, and adapt functions where needed. Could this be a possibility for you to include in your documentation?

We have added these in the "WAVI setup" (now named "API") section of the docs for the key methods (Simulation, Model, TimesteppingParams, OutputParams, Grid, Params), using just the autogenerated internal docs. Does this look like what you had in mind? Happy to discuss further! Note that the way we have coded the InitialConditions and SolverParameters functions (using a slightly different structure, which doesn't affect user experience) precludes us from doing this. For those two, we have just described the constructors within the code above.

5) Great, easy-to-follow paper with a fantastic overview of current ice sheet modelling theory and available models. Just a couple of comments from me on this...

L8: I would say "lower the bar" is quite a colloqial term. Please change this to something more like "WAVI.jl is designed to make ice sheet modelling more accessible to beginners and low-level users..." L25: What does the bracketed part of "On the long [O(1000s km)]..." mean" L25: "lengthscales" >> "length scales"

Thanks for these suggestions. In the updated paper, we have amended the 'lower the bar' statement as suggested (in L8 and in the final sentence), clarified that the bracketed part refers to "on the order of 1000s of kilometers", and fixed the "length scales" typo.

@JordiBolibar

During installation, following your guidelines, I encountered the following warning... └ There seems to be an issue with the package SparseArrays.jl, which as I will discuss in the next section, also hinders the use of your package.

I have tried running some of your examples (both the one in the README and the simplest one in the docs), but the code seems to crash when initializing the grid:

I would say this is by far the most important issue to address in the review. Probably it's just a minor technical issue with the package, but it would be important to have everything running smoothly before widely sharing the library with the community.

Thanks for highlighting this issue. As outlined in the previous comment (https://github.com/openjournals/joss-reviews/issues/5584#issuecomment-1764952630), this was caused by incompatibility with the sparse arrays package in newer versions of julia, which were untested and should have now been fixed.

I'm not familiar with the format of the authors in JOSS, but to me in the manuscript there seems to be 3 authors, and in the repository and the code there seems to be mainly two authors. Is this normal? Probably I'm misunderstanding something here, so apologies if I got this wrong.

It's true that most of the code has been written by myself and Rob, but other authors have gave active project direction and other forms of non-code contributions. We are happy to explicitly list these if needed (maybe @observingClouds can advise?)

I see this was also mentioned by @PennyHow, but I found it really hard to find the docs. Actually, I randomly came across them some weeks ago, but when I tried to look for the link in the repository during the review I just couldn't find it. I think a link and a mention of the docs should be clearly stated in the README (perhaps with a docs badge), and/or in the repository description.

Sorry about this. As we outlined in our response to @PennyHow, we've now added a link to the docs in the 'about' section of the repo and in the first line of the readme.

The WAVI Setup section name is a little bit confusing to me. I feel it would be clearer to call it "API" or "WAVI data structures". Setup sounds more like configuration or installation to me.

We have now renamed this section to API, thanks.

The image showing the data structures hierarchy is not displaying in the docs. It would be nice to fix this, since I'm sure it's really useful to users.

Thanks for spotting this, this is now fixed.

The simulations section has some formatting issues, where some code is displayed as a title.

Thanks for spotting this, this has now been corrected.

I find the term "melt rates" a bit vague. After reading the docs one can see that you are talking about parametrizations of ice shelf basal melt. Nonetheless, there are a lot of different physical processes involving melt on ice sheets (e.g. surface mass balance, basal mass blance, internal mass balance...). Perhaps being a little bit more specific would make things clearer for users.

That's true, and sorry for the confusion. We've updated this to "basal melt rate parametrizations".

This is quite irrelevant, but I find the format of the references to be a little bit crude.

We agree. Since we first formatted these, the DocumenterCitations package has added new formatting options (yay!) and we've adopted a numeric format now.

In the intro of the docs, it should read "at its heart" instead of "at it's heart".

Thanks for spotting this typo, we've now fixed it.

I think it's very nice that you included so many different examples of increasing complexity. This really will help users to get started and understand the internals of the model. One little thing I'd fix there is the colorbar of the heatmaps. It's an annoying common issue in Julia, where you need to manually add an extra space in order to avoid an overlap between the title and the units.

We're on this, and it will be fixed before the rest of the reviews come in.

You have a very nice logo, why not add it to the README? 🙂

We have added the logo to the README :)

Overall very nicely written and straight to the point. I really have almost no comments there, I think you managed to explain the field of ice sheet modelling to a wide audience, while providing enough details.

Thanks :)

You show examples of other models, but you don’t really mention what are the main differences between them. It is clear that WAVI.jl is more user-friendly, but perhaps it would be interesting to know a little bit more on why it is different or similar to other models.

This is a bit tricky because it's hard to do this in a non-technical way and there are a number of different models to compare to. We've added a note that the interested reader can find details in Cornford et al. (2020) (https://doi.org/10.5194/tc-14-2283-2020), in which a brief comparison between most of the major ice sheet models can be found.

@daniel-cheng

General checks The submission fulfills most checklist items, barring reproducibility of the results. I encountered similar issues as my fellow reviewers in attempting to run the code, which I was not able to resolve on my own. Functionality and performance of the code are not currently verified barring resolution of the code issues mentioned above.

Thanks @daniel-cheng. As outlined above, this is now fixed and it should now run as expected!

There is a lack of clear contributing guidelines that should be added to the repository Read Me.

We have added a contributors guide in the documentation.

There are tests, but not a clear way to run them in the publicly available documentation (as mentioned by the other reviewers at https://rjarthern.github.io/WAVI.jl/)

We are working on this, and will add this to the documentation once the rest of the reviews come in.

The manuscript is clear and concise, and shows how the development of this Julia based ice sheet flow model enhances accessibility and scientific study by providing tools for the cryospheric community.

Thanks :)

observingClouds commented 10 months ago

Dear @alextbradley, Thank you very much for addressing most of the reviewers comments and give us an update on the outstanding items, which I list for completeness and a better overview below:

[x] The colorbar of the heatmaps. It's an annoying common issue in Julia, where you need to manually add an extra space in order to avoid an overlap between the title and the units.
[x] Reproducibility of the results and general execution of the software
[x] There are tests, but not a clear way to run them in the publicly available documentation
[x] Regarding the mis-match of authors on the paper and the repository: according to our guidelines this is not an issue if these contributors provided "active project direction and other forms of non-code contributions" which @alextbradley confirmed for this submission. Please note however that the author list of the final archive (e.g. zenodo release) has to agree with the list of authors on the JOSS paper.

Dear @PennyHow, @JordiBolibar, @daniel-cheng, With the installation and usage issues potentially being fixed, I kindly ask you to test WAVE.jl's reproducability once more. Please also tick off all remaining checklist items if they were sufficiently addressed by @alextbradley's response.

Thank you all for your patience. I hope to make a final decision on this submission in two weeks time after getting your final evaluation.

Cheers, Hauke

PennyHow commented 10 months ago

Hi @alextbradley,

It works! Thanks for making those changes so that the examples work now. It's a very simple and straightforward set-up process. Just a couple of things from me now:

Can you please make a PR and consolidate everything to the main branch. I see there are branches that contain the appropriate changes, but it's best to have them on the main branch as this is where users are first directed to.
I also see that your Github pages build is held within a branch of WAVI.jl - it is best practise to limit branches to feature developments rather than entirely new projects/applications. I would advise moving the gh-pages branch to a new repo. But this is not mandatory... theroetically you can use branches for anything!
I've made some changes to your README as I found a typo and thought the Getting Help and Contributing sections looked a little empty. Look at my PR. Feel free to add more if needed. And then remember to merge the upstream branch to main
With regards to documentation and the API section of the docs, I think this will suffice. It's not exactly what I had in mind, but I think it adequately describes how each aspect of WAVI.jl and its functionality operates.

Once these changes are made then I am happy to accept, @observingClouds

openjournals / joss-reviews

[REVIEW]: WAVI.jl: Ice Sheet Modelling in Julia #5584

Status

Reviewer instructions & questions

Checklists

Review checklist for @daniel-cheng

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @PennyHow

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @JordiBolibar

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review for WAVI.jl

Installation

Running the code

Authors

Documentation

Access

API

Physics

References

Random

README

Manuscript

Comparison to the field

General checks

Functionality

Documentation

Software paper