Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JOSS Review] questions + suggestions #446

Closed
vavrines opened this issue Feb 19, 2024 · 7 comments
Closed

[JOSS Review] questions + suggestions #446

vavrines opened this issue Feb 19, 2024 · 7 comments
Labels
documentation 📚 Improvements or additions to documentation joss review 📖 Issue arising from JOSS reviewers or their comments

Comments

@vavrines
Copy link

Hi @milankl
I’m working on the review for JOSS. In addition to the checklist, here are some of my questions and suggestions:

  • Three sets of equations are introduced at the beginning of the Summary, where the first two are explained with figures. Maybe it makes sense to do the same for the barotropic vorticity equation?
  • In the manuscript, both the physical models, e.g., the atmospheric general circulation model, and the computer programs, e.g., the Dedalus, CLIMLAB, and Oceananigans, are called “models”. The latter may be renamed to avoid ambiguity.
  • The performance claims of the software appear to be limited. As the weather forecast is a pretty time-consuming task, the audience would be interested to know about the HPC capacity (distributed computing, GPU, scaling, etc.).
  • The parameterization and data-driven parts of the documentation are yet to be completed. Is this intentional?
  • It is mentioned that “The primitive equation core (wet or dry) is in development” in the README. How complete is this development?
@vavrines
Copy link
Author

@milankl
Copy link
Member

milankl commented Feb 21, 2024

Thanks @vavrines, much appreciated, we'll be working on those and let you know when and how they're addressed!

@milankl milankl added joss review 📖 Issue arising from JOSS reviewers or their comments documentation 📚 Improvements or additions to documentation labels Feb 21, 2024
@navidcy
Copy link
Collaborator

navidcy commented Feb 22, 2024

  • Three sets of equations are introduced at the beginning of the Summary, where the first two are explained with figures. Maybe it makes sense to do the same for the barotropic vorticity equation?

@milankl, perhaps a snapshot from the video in the README?

stochastic_stirring_T340_ortho.mp4

@milankl
Copy link
Member

milankl commented Feb 28, 2024

@vavrines Many thanks for the questions!! Some answers

Three sets of equations are introduced at the beginning of the Summary, where the first two are explained with figures. Maybe it makes sense to do the same for the barotropic vorticity equation?

We could add a vorticity snapshot as shown by Navid ☝🏼, but that's fairly similar to the shallow water equations hence I had initially left it out from the manuscript. I'd be happy to include it if you insist, but the manuscript is already rather lengthy for JOSS and maybe other variables of the primitive dry/wet model should be preferred, e.g. in #460 is a video of clouds. I don't know how many showcase figures should be in the manuscript vs just in some gallery of the repository.

In the manuscript, both the physical models, e.g., the atmospheric general circulation model, and the computer programs, e.g., the Dedalus, CLIMLAB, and Oceananigans, are called “models”. The latter may be renamed to avoid ambiguity.

That's a good call, the language around model = software vs mathematical equations is unfortunately often ambiguous. I'll rephrase this in the manuscript and reference this here for you to check.

The performance claims of the software appear to be limited. As the weather forecast is a pretty time-consuming task, the audience would be interested to know about the HPC capacity (distributed computing, GPU, scaling, etc.).

Yeah, at the moment we only have single CPU and multi-threading CPU support unfortunately. I don't think this currently heavily limits it's use cases and purposes but I agree that in the readme that's at the moment not well reflected. At the same time, I almost exclusively run SpeedyWeather on my laptop for development and reach typical speeds of 400yrs/day at 400km resolution (T31) and 4yrs/day at 100km resolution (T127) on a single CPU which is probably faster than most people expect. So yes, what about I create a simple overview table in the readme of typical speeds with over some resolutions, single and multi-threaded, across the four models we have? I don't think an in-depth analysis is needed (unless you think so) but just to give users an overview what performance they can expect and whether that fits their needs?

The parameterization and data-driven parts of the documentation are yet to be completed. Is this intentional?

Yes, I'm still working on this. I've pushed this back because we only recently changed the convection and large-scale condensation scheme and we still don't have the radiation scheme I'd like to have. Once a scheme seems to work well as a default I'll document it. Meaning I can document convection, condensation and surface fluxes next week or so but would document only the simple UniformCooling for radiation we currently have.

It is mentioned that “The primitive equation core (wet or dry) is in development” in the README. How complete is this development?

The readme indeed needs a little overhaul. The primitive equations (wet and dry) are implemented and ready for experiments. What's in development are the physical parameterizations that can be thought of as an add-on to those equations. I'll update the readme and reference this here for you to see.

@vavrines
Copy link
Author

Thanks for the answer.

some gallery of the repository

This is just a personal advice from the fact that it will be helpful for someone like me who is not in the climate community to understand better. There's no sticking to it. You may choose whatever form you think is appropriate.

what about I create a simple overview table in the readme of typical speeds

That also sounds good.

Now everything is clear to me.

Lastly, thank you for creating this work. It should be very helpful for both the scientific research and Julia community.

@milankl
Copy link
Member

milankl commented Apr 2, 2024

@vavrines Many thanks for the review, below are our responses

  1. Three sets of equations are introduced at the beginning of the Summary, where the first two are explained with figures. Maybe it makes sense to do the same for the barotropic vorticity equation?

I actually like this suggestion, so I've added this figure

image

which showcases the barotropic vorticity model but also the particle advection, we've recently added.

  1. In the manuscript, both the physical models, e.g., the atmospheric general circulation model, and the computer programs, e.g., the Dedalus, CLIMLAB, and Oceananigans, are called “models”. The latter may be renamed to avoid ambiguity.

I'm hesitating to rename the computer prorgrams to something else than model, not because I don't agree with the ambiguity here, but because the term "model" to mean a computer program that simulates some geophysical phenonmea or processes by solving equations is so common. I'd therefore rather use "equations" wherever the computer program-independent equations are meant, i.e. "shallow water equations", "barotropic vorticity equations", "primitive equations", if it's about the implementation here the word "model" is added, e.g. "The primitive equation model in SpeedyWeather.jl". Does that sound good?

  1. The performance claims of the software appear to be limited. As the weather forecast is a pretty time-consuming task, the audience would be interested to know about the HPC capacity (distributed computing, GPU, scaling, etc.).

This has been i) better reflected in the readme with #486 ii) addressed in the manuscript following a similar comment from another reviewer, see #480 (comment)

A simple parallelization (across vertical layers for the dynamical core, across horizontal grid points for the parameterizations) is supported by Julia's multithreading. No distributed-memory parallelization is currently supported, GPU support is planned.

and iii) we have added a benchmark suite with #509, see https://github.com/SpeedyWeather/SpeedyWeather.jl/tree/main/benchmark

  1. The parameterization and data-driven parts of the documentation are yet to be completed. Is this intentional?

Yes, I'm still working on this and will let you know about progress.

  1. It is mentioned that “The primitive equation core (wet or dry) is in development” in the README. How complete is this development?

This has been rewritten in the README to reflect that the development of the primitive equation models is indeed completed. Sure, there's always things we will improve over time, but it's in a usable state with many different parameterizations implemented, which allow to study a wide range of atmospheric phenomena.

@vavrines
Copy link
Author

vavrines commented Apr 3, 2024

Hi @milankl

Thanks for the input!
Now everything is clear to me. The paper is in good shape.
We can close this now. I know you'll keep improving [5]. The documentation in the dev version should be sufficient to support the publication.

@vavrines vavrines closed this as completed Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📚 Improvements or additions to documentation joss review 📖 Issue arising from JOSS reviewers or their comments
Projects
None yet
Development

No branches or pull requests

3 participants