search

openjournals / joss-reviews

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

[REVIEW]: FlowSieve: A Coarse-Graining Utility for Geophysical Flows on the Sphere #4277

Closed editorialbot closed 1 year ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@bastorer<!--end-author-handle-- (Benjamin Storer) Repository: https://github.com/husseinaluie/FlowSieve Branch with paper.md (empty if default branch): Version: v3.3.0 Editor: !--editor-->@kthyng<!--end-editor-- Reviewers: @NoraLoose, @ashwinvis Archive: 10.5281/zenodo.7818192

Status

status

Status badge code:

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

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

@NoraLoose & @kris-rowe, 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 @kthyng 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 @NoraLoose

📝 Checklist for @ashwinvis

kthyng commented 1 year ago

Hi @kris-rowe and @NoraLoose! Any updates on your reviews?

NoraLoose commented 1 year ago

Hi @kthyng! I continued my review, and opened 5 new issues 2 days ago (see above). I think these have to be addressed before I can check off the remaining boxes in the review checklist. Thanks!

kthyng commented 1 year ago

Thanks @NoraLoose. @bastorer please check out and address the comments to keep this moving forward.

@kris-rowe Will you be able to return to your review soon? Thanks.

bastorer commented 1 year ago

Hi @kthyng I've submitted a couple commits and text responses to the GitHub issues that @NoraLoose posted on the FlowSieve page (https://github.com/husseinaluie/FlowSieve/issues).

NoraLoose commented 1 year ago

@editorialbot generate pdf

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:

kthyng commented 1 year ago

@kris-rowe Hi! I think now is a good time to assess your review and decide if you'll be able to complete it before Thanksgiving. I know it's a busy time for everyone, and if you won't be able to finish I would prefer you let me know so I can recruit a replacement reviewer and get this all finalized. Thanks for your effort.

kthyng commented 1 year ago

Hi @bastorer — here is an update. kris-rowe won't be able to finish the review, so I will recruit a new reviewer. I am sorry about this setback.

kthyng commented 1 year ago

@editorialbot remove @kris-rowe from reviewers

editorialbot commented 1 year ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@editorialbot commands

kthyng commented 1 year ago

@editorialbot remove @kris-rowe from reviewers

editorialbot commented 1 year ago

@kris-rowe removed from the reviewers list!

kthyng commented 1 year ago

Hi @ashwinvis! We have a submission to JOSS currently underway in review, and need a new reviewer. Any chance you have the time to step in and do this review? It'd be great to get it wrapped up by the end of the year. Thanks for your consideration.

ashwinvis commented 1 year ago

Hi @kthyng. Looks interesting, I can take a look, but I won't be able to do it this month. I have some other commitments. I could do it January.

kthyng commented 1 year ago

@bastorer I know it's not ideal but I think it's unlikely we'll find someone with the time to review your software before potentially breaking for holidays. However, it's great that we have a second reviewer lined up for the new year now!

Also, I was just looking at your repo. I think it'd be good for your software to have more information in the readme about what it is for — currently it is about citations, etc, which is good info to have there too. But I'd suggest prefacing that information with a brief description of what it does.

kthyng commented 1 year ago

@ashwinvis That is great, thank you! I will add you as a reviewer so you can get started next month, and I'll check in with you then.

@NoraLoose Might you be able to wrap up your review in the meantime? Thanks for your consistent effort on this.

kthyng commented 1 year ago

@editorialbot add @ashwinvis as reviewer

editorialbot commented 1 year ago

@ashwinvis added to the reviewers list!

kthyng commented 1 year ago

Oh and @ashwinvis when you do start your review, post a comment with @editorialbot generate my checklist to generate your reviewer checklist.

NoraLoose commented 1 year ago

Hi @kthyng, here is a summary of the issues that @bastorer and I have worked on over the past few months:

I think we are pretty close. I am just waiting on some last fixes from Ben concerning the remaining 2 open issues. I'm happy if we can wrap it up before the break.

kthyng commented 1 year ago

Thanks for that excellent summary!

NoraLoose commented 1 year ago

Hi @bastorer and @kthyng, just as a heads-up: I will go on maternity leave in February, but really whenever the baby decides to show up.

Ben, it would be great if we could wrap up my part of the review in the next 2 weeks. I think we are very close! :)

kthyng commented 1 year ago

Congrats @NoraLoose! Yes let's get your part completely wrapped up. Looks like there are 2 open issues related to 3 unchecked review boxes to finish.

@ashwinvis When will you be able to start your review?

Happy New Year all!

bastorer commented 1 year ago

@NoraLoose That's very exciting, congratulations! Absolutely, I'm working through the conda environments right now :-)

kthyng commented 1 year ago

Hi @ashwinvis! Please check in and let us know when you'll be starting your review.

ashwinvis commented 1 year ago

Review checklist for @ashwinvis

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

bastorer commented 1 year ago

Hi @ashwinvis

Just checking if this is an in-progress checklist, or if the unchecked items are ones where you have concerns about FlowSieve.

Thanks!

NoraLoose commented 1 year ago

Hi @kthyng, @bastorer!

All issues have been addressed, and I checked the remaining boxes in the review list. I am happy for the software to be published in JOSS. :tada:

ashwinvis commented 1 year ago

Hi @ashwinvis

Just checking if this is an in-progress checklist, or if the unchecked items are ones where you have concerns about FlowSieve.

Thanks!

Working on it. I did notice that the installation docs are insufficient. It does not list a list of external dependencies and libraries required (for eg. Netcdf, I dont know if there are more).

ashwinvis commented 1 year ago

@bastorer I have added two issues https://github.com/husseinaluie/FlowSieve/issues/23 https://github.com/husseinaluie/FlowSieve/issues/24

kthyng commented 1 year ago

Hi @ashwinvis and @bastorer — am I correct in my reading that progress is currently waiting on https://github.com/husseinaluie/FlowSieve/issues/24? And can I just say, ugh, compiler flags?

bastorer commented 1 year ago

Hi @kthyng I don't have access to test / build with gcc10, but it sounds like @ashwinvis was able to compile with gcc9 (I've tested with 8 and 9 on my end, and they worked as expected).

@ashwinvis did you run into any other problems with the software? Until I can find a gcc10 environment to work in, I may just need to add to the minimum requirements (husseinaluie/FlowSieve#23) that gcc10+ are currently not supported.

kthyng commented 1 year ago

@bastorer and @ashwinvis It does seem like if the code can compiled with gcc8 and gcc9, then gcc10 can be left for future work. @ashwinvis do you agree? What are the next steps in your review?

ashwinvis commented 1 year ago

@kthyng I agree, as long as it is explicitly mentioned in the docs that FlowSieve only supports upto gcc9 (as @bastorer says he will) we can move forward. I noted now in https://github.com/husseinaluie/FlowSieve/issues/24 that it works as expected with gcc9.

ashwinvis commented 1 year ago

@bastorer I have the following annotations from the manuscript

“careful coarse-graining” (Storer and Aluie, 2022, p. 1) (pdf) I wonder if this adjective mean anything in this context?

“python user-friendliness” (Storer and Aluie, 2022, p. 1) (pdf) reword as "user-friendly Python"

“extend this functionality” (Storer and Aluie, 2022, p. 1) (pdf) to what kind of grids?

“State of the Field” (Storer and Aluie, 2022, p. 1) (pdf) The state of the field section only mentions what other packages exists. However to the reader it remains unclear what advantages or disadvantages FlowSieve bring in compared to other coarse-graining approaches/

“python” (Storer and Aluie, 2022, p. 2) (pdf) capitalize "Python"

“Another approach applies structure functions to Lagrangian trajectories to extract spectral diagnostics, such as power spectra and inter-scale energy transfers (Frisch (1995), Balwada et al. (2022)).” (Storer and Aluie, 2022, p. 2) (pdf) How does computation of spectra and transfer functions relate to coarse-graining? To my experience these are statistical quantities. One cannot build a vector field from say a K.E. spectra, only vice-versa is possible. This statement seems out-of-context here, so perhaps move it to another paragraph.

“follows the rigorous underlying mathematical framework to preserve physical properties of the data” (Storer and Aluie, 2022, p. 2) (pdf) Which mathematical framework? Which physical properties? Please specify and / or cite.

bastorer commented 1 year ago

Thanks for these notes! I've updated the paper files and pushed them to the repo :-)

“careful coarse-graining” (Storer and Aluie, 2022, p. 1) (pdf) I wonder if this adjective mean anything in this context?

"Careful" does have some meaning, since 'direct' filtering of the velocity fields without Helmholtz decomposition [whether with coarse-graining or block averaging (which is also sometimes called coarse-graining)] does not guarantee commutativity with differential operators. In that sense, by including the Helmholtz decomposition step, we are being more careful in our scale decomposition.

“python user-friendliness” (Storer and Aluie, 2022, p. 1) (pdf) reword as "user-friendly Python"

Text updated.

“extend this functionality” (Storer and Aluie, 2022, p. 1) (pdf) to what kind of grids?

Text updated to specify "(e.g. to include unstructured grids)"

“State of the Field” (Storer and Aluie, 2022, p. 1) (pdf) The state of the field section only mentions what other packages exists. However to the reader it remains unclear what advantages or disadvantages FlowSieve bring in compared to other coarse-graining approaches

That's a fair point. The main advantages of FlowSieve are 1) designed to handle spherical geometries [i.e. can filter full global flow fields] 2) computationally / numerically stable at all filter scales, so can coarse-grain at all scales from grid-scale to domain-scale [e.g. Storer et al., 2022 filters from 10s of km up to 40,000km (the equatorial circumference of the Earth)] 3) heavily parallelized to utilize HPC environments 4) on-line post-processing and diagnostic calculations [e.g. across-scale energy transfers].

I will update the text accordingly!

“python” (Storer and Aluie, 2022, p. 2) (pdf) capitalize "Python"

Text updated.

“Another approach applies structure functions to Lagrangian trajectories to extract spectral diagnostics, such as power spectra and inter-scale energy transfers (Frisch (1995), Balwada et al. (2022)).” (Storer and Aluie, 2022, p. 2) (pdf) How does computation of spectra and transfer functions relate to coarse-graining? To my experience these are statistical quantities. One cannot build a vector field from say a K.E. spectra, only vice-versa is possible. This statement seems out-of-context here, so perhaps move it to another paragraph.

Computing power spectra and transfer functions are actually our primary applications for coarse-graining. Coarse-graining is applied to the physical-space vector fields to compute the amount of energy contained in scales larger than a given ell. By then performing a 'scan' over a wide range of filter scales ell, we can determine the contribution to the total energy from each scale, and thereby compute the KE spectrum. In Storer et al., 2022 we used this method to compute power spectra on the entire globe. Aluie et al. 2018, among others, used coarse-graining to compute the across-scale kinetic energy transfer.

This statement is included to acknowledge other techniques that have been implemented to try and measure similar flow properties (spectra, energy transfers, etc) as coarse-graining.

“follows the rigorous underlying mathematical framework to preserve physical properties of the data” (Storer and Aluie, 2022, p. 2) (pdf) Which mathematical framework? Which physical properties? Please specify and / or cite.

Because of the methodology, we preserve commutativity with differential operators. For physical properties, that means that a flow that is initially non-divergent will remain non-divergent. Also that computing the vorticity of a filtered field is equivalent to computing the filtered vorticity. Those properties are not guaranteed with direct 'block averaging' or filtering with using Helmholtz decomposition.

For the 'mathematical framework', I'm referring to Aluie 2019 (citation added to text).

ashwinvis commented 1 year ago

@editorialbot generate pdf

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:

ashwinvis commented 1 year ago

Thanks @bastorer it is much clearer. Some minor comments regarding the language. I could not overlook it since it was so obvious and they (JOSS) always put the name of reviewer on the article :)

“The unique contributions of FlowSieve to the field are:” (Storer and Aluie, p. 2) (pdf)

Now it is clear how FlowSieve sets itself apart from other solutions. Please fix the grammatical structure of this statement though. Right now it conveys "to the field are + Designed for" (also make it lowercase d) and  "to the field are + can apply any", which sounds wrong. Also replace periods between each items with a comma.

“cirfumerence” (Storer and Aluie, p. 2) (pdf)

typo

“to 51 preserve physical properties of the data (e.g. non-divergence of flow) and to accurately filter 52 flows that not divergence-free.” (Storer and Aluie, p. 2) (pdf)

Thanks for clarifying, but repeats "non-divergence"

ashwinvis commented 1 year ago

More importantly, while trying to check out

Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?

it occured to me that it is unclear, both from the article and the documentation, what the core functionality is. In other words which is the public API for others? Is it a C++ library or a selection of command line utilities that you have put together under Case_Files? If it is the latter, as it appears from the tutorials, it should be documented

kthyng commented 1 year ago

Looks like we need some input from @bastorer here!

bastorer commented 1 year ago

Thanks @bastorer it is much clearer. Some minor comments regarding the language. I could not overlook it since it was so obvious and they (JOSS) always put the name of reviewer on the article :)

Absolutely! I'm always happy to hear suggestions on improved wording :-)

I've updated the text in the most recent commit (fbdc32bb), but copied the update text below as well.

The unique contributions of FlowSieve to the field are: 1) analysis on full spherical geometries, allowing the processing of global data, 2) arbitrary filtering scales spanning from sub-grid to domain-size (e.g. > @Storer2022 extracts global power spectra for scales spanning 10s of km to 40000 km - the equatorial circumference of the Earth), 3) on-line diagnostic calculations [e.g. across-scale energy transfers, large-scale vorticity and divergence], 4) on-line post-processing to reduce output file sizes [e.g. averages over user-specified regions, zonal averages], 5) rigorous underlying mathematical framework of @Aluie2019 to preserve physical properties of the data (e.g. non-divergence of flow) to accurately filter realistic flows and ensure commutativity between filtering and differential operators, and 6) heavy parallelization to utilize HPC environments efficiently.

bastorer commented 1 year ago

it occured to me that it is unclear, both from the article and the documentation, what the core functionality is. In other words which is the public API for others? Is it a C++ library or a selection of command line utilities that you have put together under Case_Files? If it is the latter, as it appears from the tutorials, it should be documented

I'm not entirely sure that I understand the difference. The use-case that I imagine is that someone would install FlowSieve, compile the appropriate executables (e.g. Helmholtz_projection.x and coarse_grain_Helmholtz.x from Case_Files), and run those executables on whatever dataset of interest they have.

Depending on the diagnostics that they want to study, they would possible add those routines to Functions [although admittedly that would require a fair bit of understanding of the code structure, and so this might more realistically involve the user reaching out to us to help them set up their new diagnostics].

So, it sounds like

selection of command line utilities that you have put together under Case_Files

is the most accurate description, I guess I'm just not sure how that should be reflected in the documentation. Could you clarify / advise?

Thanks for all of your help!

ashwinvis commented 1 year ago

For example, if you look at

It says "you generally just need to appeal to two audiences: Users & Developers"

Xarray does a good job in that. https://docs.xarray.dev/en/stable/

In your case you expect users to compile the executables in Case Files and use them as command line utilities. And developers can use it as a C++ library. This should be made clear prominently in the docs.

bastorer commented 1 year ago

Thanks for clarifying! I've added some notes to the documentation. Specifically, on the documentation landing page I added

Usage

  • Users can expect to compile the executables in 'Case Files' and use them as command-line utilities to process existing netCDF-4 data. The tutorials illustrate the steps required for this usage, as well as highlighting the kind of outputs / analysis that can be obtained.
  • Developers can use FlowSieve as a C++ library and develop additional diagnostic / analysis routines using the FlowSieve codebase.
kthyng commented 1 year ago

Just checked in and see that @ashwinvis has one checkbox remaining and there is an active conversation at https://github.com/husseinaluie/FlowSieve/issues/29.

ashwinvis commented 1 year ago

Yes @kthyng I just need to check if MPI works.

ashwinvis commented 1 year ago

@kthyng I have finished the review and I conclude that FlowSieve is a high-performance and potentially reusable tool for downscaling and / or analysing data from various geophysical applications. All the claims in the manuscript has been verified against the software implementation and I recommend that the article may be accepted into JOSS.

Congrats @bastorer

bastorer commented 1 year ago

@kthyng let me know if there's anything that I can do to help finalize the review, etc.

Thanks for all of your help with the review process!

kthyng commented 1 year ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance