[REVIEW]: GeophysicalFlows.jl: Solvers for geophysical fluid dynamics problems in periodic domains on CPUs & GPUs

whedon commented 3 years ago

Submitting author: @navidcy (Navid C. Constantinou) Repository: https://github.com/FourierFlows/GeophysicalFlows.jl Version: v0.12.1 Editor: @pdebuyl Reviewer: @ranocha, @eviatarbach Archive: 10.5281/zenodo.4695260

: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 badge code:

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

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

@ranocha & @eviatarbach, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

Make sure you're logged in to your GitHub account
Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @pdebuyl know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @ranocha

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 repository url?
[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 (@navidcy) 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

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: Does the paper have a section titled 'Statement of Need' that clearly states 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: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[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?

Review checklist for @eviatarbach

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 repository url?
[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 (@navidcy) 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

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: Does the paper have a section titled 'Statement of Need' that clearly states 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: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[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?

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @ranocha, @eviatarbach 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:

Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

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 3 years ago

Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.20 s (232.0 files/s, 43355.3 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Julia                           25           1721            498           4898
Markdown                        13            419              0           1048
YAML                             6             17              8            115
TOML                             3              6              0             53
-------------------------------------------------------------------------------
SUM:                            47           2163            506           6114
-------------------------------------------------------------------------------

Statistical information for the repository 'bb0dae94f28ce15b0204c8e1' was
gathered on 2021/02/22.
No commited files with the specified extensions were found.

whedon commented 3 years ago

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

Can't find any papers to compile :-(

pdebuyl commented 3 years ago

@ranocha @eviatarbach make sure to accept the invitation to the reviewers group and to have a look at the reviewer guidelines linked to at the top of this review page.

The review process will happen in this issue page, so questions to the author or to me can be added as comments here.

pdebuyl commented 3 years ago

@whedon generate pdf from branch JOSS-paper

whedon commented 3 years ago

Attempting PDF compilation from custom branch JOSS-paper. Reticulating splines etc...

whedon commented 3 years ago

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

eviatarbach commented 3 years ago

Before getting into the rest of the review, I ran into some installation issues on older versions of Julia. On Julia 1.5.3 the installation works fine.

On Julia 1.4.1 (less than a year old), the error https://github.com/FourierFlows/GeophysicalFlows.jl/issues/128 occurs upon first import. I understand this was fixed in GeophysicalFlows.jl version 0.8.1, but that version requires Julia 1.5.0, meaning that the package installation is broken on Julia 1.4.1 (and I assume all versions of Julia between 1.4.0 and 1.5.0). I suggest that either a fix is backported or the package be marked as incompatible with these versions.

I also tried installing it on Julia 1.3.1, which results in an error about unsatisfiable requirements. This one is perhaps not as important because it gives an explicit error at installation time. However, I think it would be good to determine which versions of Julia this package is compatible with at present (especially versions since Julia 1.0.5, the LTS), and state this in the repository README.

navidcy commented 3 years ago

Thanks @eviatarbach for pointing this out. I’ll try to sort it out and ping you.

navidcy commented 3 years ago

Regarding the error on Julia v1.4, I could fix the issue if I could edit a file from the v0.8.0 release. how do I do that? or what should I google to figure it out?

pdebuyl commented 3 years ago

@navidcy you can issue a bugfix release 0.8.5 if necessary. Google should advise you not to edit the content of the release :-)

Is it necessary to fix older versions for this issue?

navidcy commented 3 years ago

hi @eviatarbach, regarding the issue with Julia v1.4.2, I've pushed a bugfix release v0.8.5 which should be fine.

julia> using GeophysicalFlows
[ Info: Precompiling GeophysicalFlows [44ee3b1c-bc02-53fa-8355-8e347616e15e]
[ Info: FourierFlows will use 12 threads
[ Info: FourierFlows will use 12 threads

julia>

and


(@v1.4) pkg> test GeophysicalFlows
    Testing GeophysicalFlows
Status `/private/var/folders/1z/s8gw4q055514jw75ztnclx4h0000gn/T/jl_Mqed4V/Manifest.toml`
  [621f4979] AbstractFFTs v1.0.1
  [79e6a3ab] Adapt v2.4.0
  [56f22d72] Artifacts v1.3.0
  [13072b0f] AxisAlgorithms v1.0.0
  [b99e7846] BinaryProvider v0.5.10
  [fa961155] CEnum v0.4.1
  [052768ef] CUDA v1.3.3
  [d360d2e6] ChainRulesCore v0.9.29
  [944b1d66] CodecZlib v0.7.0
  [34da2185] Compat v3.25.0
  [e66e0078] CompilerSupportLibraries_jll v0.3.4+0
  [a2441757] Coverage v1.2.0
  [c36e975a] CoverageTools v1.2.2
  [864edb3b] DataStructures v0.18.9
  [ffbed154] DocStringExtensions v0.8.3
  [e2ba6199] ExprTools v0.1.3
  [7a1cc6ca] FFTW v1.3.2
  [f5851436] FFTW_jll v3.3.9+7
  [2aec4490] FourierFlows v0.6.8
  [0c68f7d7] GPUArrays v5.2.1
  [61eb1bfa] GPUCompiler v0.6.1
  [44ee3b1c] GeophysicalFlows v0.8.5
  [cd3eb016] HTTP v0.9.5
  [83e8ac13] IniFile v0.5.0
  [1d5cc7b8] IntelOpenMP_jll v2018.0.3+2
  [a98d9a8b] Interpolations v0.13.1
  [033835bb] JLD2 v0.3.3
  [692b3bcd] JLLWrappers v1.2.0
  [682c06a0] JSON v0.21.1
  [929cbde3] LLVM v2.0.0
  [4af54fe1] LazyArtifacts v1.3.0
  [856f044c] MKL_jll v2021.1.1+1
  [1914dd2f] MacroTools v0.5.6
  [739be429] MbedTLS v1.0.3
  [c8ffd9c3] MbedTLS_jll v2.16.8+1
  [872c559c] NNlib v0.7.14
  [ca575930] NetworkOptions v1.2.0
  [6fe1bfb0] OffsetArrays v1.6.1
  [efe28fd5] OpenSpecFun_jll v0.5.3+4
  [bac558e1] OrderedCollections v1.4.0
  [69de0a69] Parsers v1.0.15
  [c84ed2f1] Ratios v0.4.0
  [189a3867] Reexport v0.2.0
  [ae029012] Requires v1.1.2
  [276daf66] SpecialFunctions v1.3.0
  [90137ffa] StaticArrays v0.12.5
  [a759f4b9] TimerOutputs v0.5.8
  [3bb67fe8] TranscodingStreams v0.9.5
  [5c2747f8] URIs v1.2.0
  [efce3f68] WoodburyMatrices v0.5.3
  [83775a58] Zlib_jll v1.2.11+18
  [2a0f44e3] Base64
  [ade2ca70] Dates
  [8bb1440f] DelimitedFiles
  [8ba89e20] Distributed
  [b77e0a4c] InteractiveUtils
  [76f85450] LibGit2
  [8f399da3] Libdl
  [37e2e46d] LinearAlgebra
  [56ddb016] Logging
  [d6f4376e] Markdown
  [a63ad114] Mmap
  [44cfe95a] Pkg
  [de0858da] Printf
  [3fa0cd96] REPL
  [9a3f8284] Random
  [ea8e919c] SHA
  [9e88b42a] Serialization
  [1a1011a3] SharedArrays
  [6462fe0b] Sockets
  [2f01184e] SparseArrays
  [10745b16] Statistics
  [8dfed614] Test
  [cf7118a7] UUIDs
  [4ec0a83e] Unicode
[ Info: FourierFlows will use 12 threads
testing on CPU
Test Summary: | Pass  Total
Utils         |    2      2
Test Summary:    | Pass  Total
TwoDNavierStokes |    9      9
Test Summary: | Pass  Total
BarotropicQG  |   18     18
Test Summary:  | Pass  Total
BarotropicQGQL |   14     14
Test Summary: | Pass  Total
SurfaceQG     |    9      9
testing on GPU
Test Summary: | Pass  Total
Utils         |    2      2
┌ Warning: calls to Base intrinsics might be GPU incompatible
│   exception =
│    You called cos(x::T) where T<:Union{Float32, Float64} in Base.Math at special/trig.jl:100, maybe you intended to call cos(x::Float64) in CUDA at /g/data/v45/nc3020/.julia/packages/CUDA/dZvbp/src/device/intrinsics/math.jl:5 instead?
│    Stacktrace:
│     [1] cos at special/trig.jl:100
│     [2] broadcast_kernel at /g/data/v45/nc3020/.julia/packages/GPUArrays/uaFZh/src/host/broadcast.jl:60
└ @ GPUCompiler /g/data/v45/nc3020/.julia/packages/GPUCompiler/GKp4B/src/irgen.jl:68
Test Summary:    | Pass  Total
TwoDNavierStokes |    9      9
Test Summary: | Pass  Total
BarotropicQG  |   18     18
Test Summary:  | Pass  Total
BarotropicQGQL |   14     14
Test Summary: | Pass  Total
SurfaceQG     |    9      9
Test Summary: | Pass  Total
MultilayerQG  |   14     14
Total test time: 1111.3896864
    Testing GeophysicalFlows tests passed

navidcy commented 3 years ago

@eviatarbach, regarding supporting Julia's longterm release v1.0.5, we can't be using CUDA.jl (since even v0.1.0 requires Julia v1.3). Unfortunately, the latest GeophysicalFlows.jl version that is supported on Julia v1.0.5 is only v0.5.1.

navidcy commented 3 years ago

I have added remark regarding version compatibilities with Julia's releases in the readme; see 1fe5dfc.

pdebuyl commented 3 years ago

@navidcy typically a JOSS reviews considers a single version of the software. The version listed upon submission is v0.11.3, may I ask what is the relevance of updating earlier versions?

eviatarbach commented 3 years ago

@navidcy Thank you for addressing this issue! It works now on Julia 1.4 for me as well.

@pdebuyl I brought up this issue. I thought that this would be important to address since the installation was failing on even quite recent versions of Julia. I apologize if this was outside the scope of the review.

navidcy commented 3 years ago

@pdebuyl, as @eviatarbach explained I was trying to address their remark above. I felt that adding a note in the readme was indeed very appropriate.

pdebuyl commented 3 years ago

@eviatarbach this was not out of scope, no problem. I did not get why there was a discussion of older versions of the code but it all makes sense. + There is now a bugfix release and extra information in the README, which is good.

ranocha commented 3 years ago

Thanks for creating this nice package (and the basic functionality in FourierFlows.jl), @navidcy et al. Please find below some comments and suggestions.

[x] You should consider reformatting the license file LICENSE.md such that GitHub recognizes the license type automatically, cf. https://opensource.org/licenses/MIT
[x] It would be good to have a top-level CONTRIBUTING file (mentioned also in the docs) so users see a link to it when they open a PR. It would also be good to note if you prefer users create a GitHub issue or use some other means for reporting problems. Related issue: https://github.com/FourierFlows/GeophysicalFlows.jl/issues/180 Edit: Done in https://github.com/FourierFlows/GeophysicalFlows.jl/pull/204
[x] The currently latest release is v0.8.5 but the README tells people to cite v0.11.4 - that's really confusing and weird.
[x] There is no explicit section "Statement of need" required for review. It would be good to structure the paper a bit more, e.g. by including sections like "Features" and "Related research and software" (although some related information is contained in the manuscript already).
[x] Would it be possible to leverage ApproxFun.jl (and/or related Matlab tools) for these problems?
[x] What is the reason for implementing the time stepping algorithms on your own instead of using something like OrdinaryDiffEq.jl?
[x] How do you think about relations to Oceananigans.jl?
[x] From reading the manuscript and the README, it's not clear to me what kind of scalability to expect. Is it correct that this package is restricted to using a single CPU or GPU, mostly in two spatial dimensions? This should be made more clear.
[x] What about integration of functionality from PencilFFTs.jl for parallelism? Would something like this be possible?
[x] I could not launch some examples in Binder as described in the documentation, see https://github.com/FourierFlows/GeophysicalFlows.jl/issues/203
[x] I also tried to run the first few examples from the documentation on the GPU but got some errors like ERROR: scalar getindex is disallowed. It would be really nice to point the reader explicitly to some GPU examples or (even better) to make the CPU examples compatible with running them on the GPU.

pdebuyl commented 3 years ago

Thank you @ranocha for the review.

The statement of need is supposed to be checked on submission, probably it wasn't because of the non "master" branch for the paper.

navidcy commented 3 years ago

@ranocha, I don't really see how I should reformat the file in view of your remark: "You should consider reformatting the license file LICENSE.md such that GitHub recognizes the license type automatically, cf. https://opensource.org/licenses/MIT"

Could you elaborate a bit perhaps?

ranocha commented 3 years ago

@ranocha, I don't really see how I should reformat the file in view of your remark: "You should consider reformatting the license file LICENSE.md such that GitHub recognizes the license type automatically, cf. https://opensource.org/licenses/MIT"

Could you elaborate a bit perhaps?

Changing LICENSE.md from

The GeophysicalFlows.jl package is licensed under the MIT "Expat" License:

> Copyright (c) 2020: Navid C. Constantinou and Gregory L. Wagner
>
> Permission is hereby granted, free of charge, to any person obtaining a copy
> of this software and associated documentation files (the "Software"), to deal
> in the Software without restriction, including without limitation the rights
> to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> copies of the Software, and to permit persons to whom the Software is
> furnished to do so, subject to the following conditions:
>
> The above copyright notice and this permission notice shall be included in all
> copies or substantial portions of the Software.
>
> THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> SOFTWARE.
>

to something like

MIT License

Copyright (c) 2020: Navid C. Constantinou and Gregory L. Wagner

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.

should help. You should basically use the text from the link I posted above with only minor modifications such that GitHub recognizes the license automatically and displays it in the summary part of the repo, e.g. instead of

navidcy commented 3 years ago

OK I see! Thanks. Done via 1439a25.

navidcy commented 3 years ago

@ranocha, regarding your remarks on

You should consider reformatting the license file LICENSE.md such that GitHub recognizes the license type automatically, cf. https://opensource.org/licenses/MIT Done; see release v0.11.5
The currently latest release is v0.8.5 but the README tells people to cite v0.11.4 - that's really confusing and weird. Done; see release v0.11.5
There is no explicit section "Statement of need" required for review. It would be good to structure the paper a bit more, e.g. by including sections like "Features" and "Related research and software" (although some related information is contained in the manuscript already). The first 3 paragraphs of the paper were aimed to summarize the statement of need. But we see your point, we'll rephrase/restructure to make it clearer to the reader!
Would it be possible to leverage ApproxFun.jl (and/or related Matlab tools) for these problems? This is something we've thought about. There is also some related discussion in https://github.com/FourierFlows/FourierFlows.jl/issues/243. But this falls outside the scope of GeophysicalFlows.jl; it should be addressed/dealt in FourierFlows.jl
What is the reason for implementing the time stepping algorithms on your own instead of using something like OrdinaryDiffEq.jl? Again, perhaps the quick short answer is "legacy; when we started FourierFlows.jl we didn't know that we could use DifferentialEquations.jl for timestepping". Later on we realized that as a possibility and it's in our mind. But again, that's outside of the scope of GeophysicalFlows.jl but rather something to be dear with within FourierFlows.jl
How do you think about relations to Oceananigans.jl? We totally forgot to remark on that in the paper! We'll do so. Several contributors are also heavily involved with Oceananigans.jl and, I guess, when we were thinking about relationship with other packages we didn't thought of Oceananigans.jl being some "other" package. :) We'll add a remark on the paper.
From reading the manuscript and the README, it's not clear to me what kind of scalability to expect. Is it correct that this package is restricted to using a single CPU or GPU, mostly in two spatial dimensions? This should be made more clear. You are correct, at the moment FourierFlows.jl is restricted to either a single GPU or a single CPU node (though multi-threading is possible and gives advantage). We'll add a remark in the README file and/or Docs.
What about integration of functionality from PencilFFTs.jl for parallelism? Would something like this be possible? Same as some of the remarks above: this is on the FourierFlows.jl to-do list and falls outside of the scope of GeophysicalFlows.jl
I could not launch some examples in Binder as described in the documentation, see FourierFlows/GeophysicalFlows.jl#203 We removed binder links since the large size of the gh-pages branch rendered the binder unusable. See https://github.com/FourierFlows/GeophysicalFlows.jl/issues/203 and https://github.com/FourierFlows/GeophysicalFlows.jl/issues/207.
I also tried to run the first few examples from the documentation on the GPU but got some errors like ERROR: scalar getindex is disallowed. It would be really nice to point the reader explicitly to some GPU examples or (even better) to make the CPU examples compatible with running them on the GPU. Fixed with https://github.com/FourierFlows/GeophysicalFlows.jl/issues/203 and release v0.11.5. Now all examples run on GPU without complaints.

(Regarding restructuring/rephrasing the paper: we'll wait to hear back from all reviewers before doing that.)

ranocha commented 3 years ago

Nice work and good updates! Please find below two additional questions/comments.

[ ] Could you please explain your reasons for creating several submodules such as TwoDNavierStokes? A more Julian approach seems to be to create some equations type and pass that around using multiple dispatch. That would probably make it easier to use GeophysicalFlows.jl as a library with multiple physical setups.
[ ] What happens if I have multiple GPUs in one workstation and choose the device GPU()?

whedon commented 3 years ago

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

whedon commented 3 years ago

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

ranocha commented 3 years ago

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

I already posted comments above.

pdebuyl commented 3 years ago

Hi @ranocha sorry for this, whedon is our automated chat bot and sometimes makes these unfortunate reminder. I'll ask about them to the JOSS team to disable them when possible.

pdebuyl commented 3 years ago

@navidcy the "Statement of need" is actually required for all JOSS papers since some time. It was not flagged for submission as the paper is not in the master branch for your repository, but you should add it. See https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain + the checklist. It should be section titled "Statement of need".

navidcy commented 3 years ago

@navidcy the "Statement of need" is actually required for all JOSS papers since some time. It was not flagged for submission as the paper is not in the master branch for your repository, but you should add it. See https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain + the checklist. It should be section titled "Statement of need".

Thanks for pointing this out -- I missed the fact that it was an actual requirement to have a section titled as such. We've looked at previous JOSS papers and followed their pattern (e.g. Oceananigans.jl) and didn't see that. But sure, we'll restructure the paper to include a Statement of need section.

navidcy commented 3 years ago

@glwagner, regarding @ranocha's question:

[ ] What happens if I have multiple GPUs in one workstation and choose the device GPU()?

do you have access to a machine with multiple GPUs to check what happens in this case?

glwagner commented 3 years ago

I will check but I believe that CUDA.jl somehow generates a list of devices (possibly in a different order than the list returned by nvidia-smi) and selects the first device of the list. It's explained in a blog post on the CUDA.jl website:

julia> using CUDA

shell> nvidia-smi
Mon Mar  8 20:39:35 2021       
+-----------------------------------------------------------------------------+
| NVIDIA-SMI 440.33.01    Driver Version: 440.33.01    CUDA Version: 10.2     |
|-------------------------------+----------------------+----------------------+
| GPU  Name        Persistence-M| Bus-Id        Disp.A | Volatile Uncorr. ECC |
| Fan  Temp  Perf  Pwr:Usage/Cap|         Memory-Usage | GPU-Util  Compute M. |
|===============================+======================+======================|
|   0  TITAN V             Off  | 00000000:18:00.0 Off |                  N/A |
| 28%   34C    P8    23W / 250W |   5150MiB / 12066MiB |      0%      Default |
+-------------------------------+----------------------+----------------------+
|   1  TITAN V             Off  | 00000000:3B:00.0 Off |                  N/A |
| 34%   48C    P0    41W / 250W |     13MiB / 12066MiB |      0%      Default |
+-------------------------------+----------------------+----------------------+
|   2  TITAN V             Off  | 00000000:86:00.0 Off |                  N/A |
| 55%   75C    P2   138W / 250W |   5826MiB / 12066MiB |     98%      Default |
+-------------------------------+----------------------+----------------------+
|   3  TITAN V             Off  | 00000000:AF:00.0 Off |                  N/A |
| 28%   41C    P8    25W / 250W |     13MiB / 12066MiB |      0%      Default |
+-------------------------------+----------------------+----------------------+

+-----------------------------------------------------------------------------+
| Processes:                                                       GPU Memory |
|  GPU       PID   Type   Process name                             Usage      |
|=============================================================================|
|    0      1551      C   julia                                       3673MiB |
|    0     10610      G   /usr/libexec/Xorg                              9MiB |
|    0     10740      G   /usr/bin/gnome-shell                          10MiB |
|    0     41030      C   /home/ballen/julia-1.5.3/bin/julia          1443MiB |
|    2     36042      C   /home/sandre/julia-1.5.2/bin/julia          5813MiB |
+-----------------------------------------------------------------------------+

julia> device()
CuDevice(0): TITAN V

julia> collect(devices())
4-element Array{CuDevice,1}:
 CuDevice(0): TITAN V
 CuDevice(1): TITAN V
 CuDevice(2): TITAN V
 CuDevice(3): TITAN V

julia> device!(1)

julia> device()
CuDevice(1): TITAN V

We could implement a convenience interface to device by adding an argument to the GPU() constructor, something like

function GPU(device_tag = device())
    device!(device_tag)
    return GPU()
end

then users could write

dev = GPU(1)

which would select device 1 for the computation.

It's also possible to limit visible devices by setting the CUDA_VISIBLE_DEVICES environment variable prior to starting julia.

ranocha commented 3 years ago

@navidcy That would be good :+1:

pdebuyl commented 3 years ago

@ranocha are you waiting for the authors for further changes? I am sorry but I can't figure properly from the discussion above if it is the case.

Regards, Pierre

ranocha commented 3 years ago

@pdebuyl Yes, exactly. I checked the boxes next to my comments that have been resolved. The other ones are still open (as far as I know).

pdebuyl commented 3 years ago

Thank you for the reply @ranocha

navidcy commented 3 years ago

@ranocha, regarding the items involving (i) ApproxFun.jl, (ii) time stepping with DifferentialEquations.jl, and (iii) using PencilFFTs.jl:

We feel that there is nothing else to do after what we replied above. But happy to discuss more on it if you think otherwise :)

eviatarbach commented 3 years ago

I commend the authors for the excellent package. I just have a few more comments before I can recommend publication:

I think you should also compare GeophysicalFlows.jl to MAOOAM and qgs in the paper, as they are also open-source packages with QG implementations.
Please separate the paper into sections, namely a 'Statement of need' section and a 'State of the field' section (see the qgs paper for an example). Currently, everything is in the 'Summary' section.
The installation instructions are not available on the online documentation, but only in the GitHub README.

Minor edit suggestions:

L17: The comma should be a colon, semicolon, or period to avoid a run-on sentence.
L29–30: "the Python's pyqg" -> "the Python package pyqg"
L34: "is Python software" -> "is a Python package"

navidcy commented 3 years ago

@eviatarbach, minor edits were taken care of by 054afc. Thanks.

pdebuyl commented 3 years ago

Thank you @eviatarbach for the review!

navidcy commented 3 years ago

@pdebuyl I plan to respond to everything and push a new version of the paper ideally in the next 2 weeks. Does this sound reasonable?

pdebuyl commented 3 years ago

@navidcy it is ok. Make sure to update us here.

navidcy commented 3 years ago

Perhaps 2 week timeline was a bit too optimistic. We might need ~10 days more.

navidcy commented 3 years ago

Response to @ranocha's remaining remarks

There is no explicit section "Statement of need" required for review. It would be good to structure the paper a bit more, e.g. by including sections like "Features" and "Related research and software" (although some related information is contained in the manuscript already).
How do you think about relations to Oceananigans.jl?

The revised manuscript now addresses both of the above remarks.

From reading the manuscript and the README, it's not clear to me what kind of scalability to expect. Is it correct that this package is restricted to using a single CPU or GPU, mostly in two spatial dimensions? This should be made more clear.
What happens if I have multiple GPUs in one workstation and choose the device GPU()?

Yeap, that's correct. FourierFlows.jl is restricted to single CPU or GPU. Multi-threading can be used to speed up ffts.

We have added a section in the package's README as well as in the documentation as well as remarks in FourierFlows.jl README and the FourierFlows.jl Docs.

Could you please explain your reasons for creating several submodules such as TwoDNavierStokes? A more Julian approach seems to be to create some equations type and pass that around using multiple dispatch. That would probably make it easier to use GeophysicalFlows.jl as a library with multiple physical setups.

Indeed, some of the modules might have overlapping functionality. For example, the equations that TwoDNavierStokes module solves is reproduced by SingleLayerQG when setting β=0, \ell = 0 and no topography. The reason we have done so was party because different disciplines feel more comfortable, i.e., mathematicians working with the 2D Navier-Stokes or 2D Euler equations don't feel quite "at home" when presented with the "single layer quasi-geostrophic equations". We see the reviewer's point thought that we could have some common functionality under the hood and then the various modules use multiple dispatch to use and/or enhance that functionality. This, though, requires a big restructure of the package and we decided to postpone it for future. Happy to discuss more on this.

Response to @eviatarbach's remaining remarks

I think you should also compare GeophysicalFlows.jl to MAOOAM and qgs in the paper, as they are also open-source packages with QG implementations.
Please separate the paper into sections, namely a 'Statement of need' section and a 'State of the field' section (see the qgs paper for an example). Currently, everything is in the 'Summary' section.

The revised paper addresses both of these remarks; see section "State of the field" in the revised version of the paper.

navidcy commented 3 years ago

@whedon generate pdf from branch JOSS-paper

whedon commented 3 years ago

Attempting PDF compilation from custom branch JOSS-paper. Reticulating splines etc...

navidcy commented 3 years ago

@pdebuyl, @ranocha, and @eviatarbach: we've pushed the revised version. Feel free to have a look.

whedon commented 3 years ago

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

eviatarbach commented 3 years ago

Thank you for addressing my points! I am happy to recommend publication.

Just a minor note: MAOOAM has Python and Lua implementations in addition to the Fortran. You also seem to have forgotten a line break before the description of MAOOAM and qgs, as opposed to the other packages.

openjournals / joss-reviews