[REVIEW]: fastSF: A parallel code for computing the structure functions of turbulence

whedon commented 4 years ago

Submitting author: @ShubhadeepSadhukhan1993 (Shubhadeep Sadhukhan) Repository: https://github.com/ShubhadeepSadhukhan1993/fastSF Version: v1.0.0 Editor: @jedbrown Reviewer: @cpgr, @iljah Archive: 10.5281/zenodo.4420031

: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/71ba620016640c5ded6e927af83f4d38"><img src="https://joss.theoj.org/papers/71ba620016640c5ded6e927af83f4d38/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/71ba620016640c5ded6e927af83f4d38/status.svg)](https://joss.theoj.org/papers/71ba620016640c5ded6e927af83f4d38)

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

@cpgr & @iljah, 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 @jedbrown know.

✨ Please try and complete your review in the next six weeks ✨

Review checklist for @cpgr

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 (@ShubhadeepSadhukhan1993) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

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: 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 @iljah

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 (@ShubhadeepSadhukhan1993) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

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: 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 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @cpgr, @iljah 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 4 years ago

Reference check summary:

OK DOIs

- 10.1093/acprof:oso/9780198722588.001.0001 is OK
- 10.1017/9781316810019 is OK
- 10.1017/CBO9781139170666 is OK
- 10.1016/C2009-0-18471-4 is OK
- 10.1098/rspa.1991.0076 is OK
- 10.1098/rspa.1991.0075 is OK
- 10.1103/PhysRevLett.70.3251 is OK
- 10.1016/j.jpdc.2017.10.014 is OK
- 10.1063/1.2001690 is OK
- 10.1063/1.1539855 is OK
- 10.1063/1.1448296 is OK
- 10.1088/1367-2630/10/3/033003 is OK
- 10.1088/1367-2630/6/1/037 is OK
- 10.1103/PhysRevE.77.016302 is OK
- 10.1017/jfm.2013.74 is OK
- 10.1146/annurev.fluid.010908.165203 is OK
- 10.1063/1.5119905 is OK
- 10.1103/PhysRevLett.72.336 is OK
- 10.1007/s12043-013-0594-4 is OK
- 10.1103/PhysRevFluids.4.084607 is OK

MISSING DOIs

- None

INVALID DOIs

- None

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

arfon commented 4 years ago

@cpgr, @iljah - please carry out your review in this issue by updating the checklist above and giving feedback in this issue. The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html

Any questions/concerns please let @jedbrown or myself know.

iljah commented 4 years ago

@jedbrown / @arfon Should title of this issue be updated to fastSF or does it matter?

arfon commented 4 years ago

@jedbrown / @arfon Should title of this issue be updated to fastSF or does it matter?

Good catch. I just updated it!

iljah commented 4 years ago

hi @ShubhadeepSadhukhan1993 , above article proof seems to contain things that aren't supposed to be there according to review guidelines ["the JOSS paper (the compiled PDF associated with this submission) should only include"] and hence should probably be removed before acceptance. @jedbrown probably knows better but I'd say the following sections should be removed: "Velocity and scalar structure functions", "Design of the Code", "Validation".

shashwat-bhattacharya commented 4 years ago

@iljah Thanks for the advice. We will do the needful and get back to you soon.

jedbrown commented 4 years ago

It's your discretion, but defining what structure functions are and the basic parallel distribution strategy is reasonable to retain (it helps a reader understand what the software really does). Validation (seems more like "verification") could be deferred to project documentation. The important point is to write for the correct audience (first impression on a potential user).

ShubhadeepSadhukhan1993 commented 4 years ago

@iljah Thanks for your view on our paper. Please mention should we retain the paper content as it is or remove the sections as mentioned earlier. We will work on them accordingly. Presently, we have removed those sections from our repository already.

ShubhadeepSadhukhan1993 commented 4 years ago

@jedbrown Please mention, what should we follow? Thanking you

jedbrown commented 4 years ago

It's your discretion based on what you think a reader will need to know to assess whether your package is likely to meet their needs.

shashwat-bhattacharya commented 4 years ago

@jedbrown Thanks for your reply. We will go ahead with your suggestion of retaining the definition of structure functions and basic parallelization strategy. We will move the validation part to the project documentation. We are currently making these changes and will update you when done.

iljah commented 4 years ago

Sorry for long delay, that sounds good. I'll try to get this done this week.

iljah commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

iljah commented 4 years ago

Current structure of pdf looks fine, two "extra" sections might provide useful background information for non-specialists. One small change is in order I think: the term strong scaling is used for describing scaling with constant work size I'd change "code exhibits strong scaling" to e.g. "code exhibits good scaling" or similar. Also, did you really use up to 1024 processors i.e. approx 512 nodes or 1024 cores and many fewer than 512 nodes? @ShubhadeepSadhukhan1993

ShubhadeepSadhukhan1993 commented 4 years ago

@iljah Thanks for your useful comments. We used fewer nodes for each run. For example, 1024 cores are achieved by using 64 nodes and 16 cores per node. We used 16 cores per node in every case.

iljah commented 4 years ago

Having trouble installing one dependency: https://github.com/anandogc/h5si/issues/1

shashwat-bhattacharya commented 4 years ago

@iljah Thanks for pointing out the issue. I just had a conversation with the developer of h5si. He will look into it and address the problem in the github page. From what I have seen in the link shared by you, the error message suggests that your c++ (CXX) compiler does not have mpi. Is it possible that you have mpi installed in your c compiler but not in your c++ compiler? Anyway, the h5si developer should be able to give you a better advice.
Thanks.

iljah commented 4 years ago

I have mpicxx and mpicc but h5si doesn't seem to be using them.

iljah commented 4 years ago

Found another issue with prerequisite h5si: https://github.com/anandogc/h5si/issues/2

cpgr commented 4 years ago

@jedbrown

I've am ready to finalise my review but cannot fill in the tick boxes, and it looks like my invite has expired. Can you please re-invite me.

Thanks!

jedbrown commented 4 years ago

@whedon re-invite @cpgr as reviewer

whedon commented 4 years ago

OK, the reviewer has been re-invited.

@cpgr please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

cpgr commented 4 years ago

Thanks @jedbrown

iljah commented 4 years ago

@ShubhadeepSadhukhan1993 I don't see any community guidelines in README as required by checklist: 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

iljah commented 4 years ago

@ShubhadeepSadhukhan1993 Could you also please add bit of info to paper about other similar packages, as required by checklist? (State of the field: Do the authors describe how this software compares to other commonly-used packages?)

iljah commented 4 years ago

And changes I suggested in https://github.com/openjournals/joss-reviews/issues/2185#issuecomment-641971304 are still relevant. At least processors should be changed to cores.

iljah commented 4 years ago

@jedbrown does performance evaluation also include scalability? Test programs included in repository are only 2D so it might be difficult to evaluate scalability claims made in article. I suggested adding 3D test program in https://github.com/ShubhadeepSadhukhan1993/fastSF/issues/2

iljah commented 4 years ago

Based on 2D tests on my laptop the stated performance seems to check out in that it would take on the order of 1000 s to calculate a 1024^2 case if my laptop had 16 cores.

jedbrown commented 4 years ago

A 3D test problem seems appropriate. Performance evaluation is whatever is claimed. So if the package makes claims about scalability, it'd be reasonable to attempt to confirm that the claims are plausible (within reason; e.g., use what hardware you have handy and that two points on a scalability curve are roughly comparable to the claims). This is mostly about making sure that someone with time/resources could run a reproduction study and as a sanity check.

ShubhadeepSadhukhan1993 commented 4 years ago

@cpgr @iljah @jedbrown Thanks for your valuable comments and suggestions. We will come back with our answers to the raised questions and also implement the suggested changes.

iljah commented 4 years ago

@ShubhadeepSadhukhan1993 to me this looks finished except for community guidelines and state of field...

shashwat-bhattacharya commented 4 years ago

@ShubhadeepSadhukhan1993 I don't see any community guidelines in README as required by checklist: 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

Thanks @iljah for the suggestion. We have added a section "Contributions and bug reports" towards the end of the README.md file, in which we briefly explain to contribute and report bugs / issues, if any.

shashwat-bhattacharya commented 4 years ago

@ShubhadeepSadhukhan1993 Could you also please add bit of info to paper about other similar packages, as required by checklist? (State of the field: Do the authors describe how this software compares to other commonly-used packages?)

@iljah We thank you for the suggestion. To the best of our knowledge and belief, no other commercial or open-source packages for computing structure functions exist or are available at the moment. Currently, such codes are developed in-house and are not open to the general public. We have added the above statements in the section "Summary" of the revised paper.

We have talked to some researchers who work on structure functions, and their codes (again, they are not open to public) typically involve 6 nested for loops (for 3D fields). However, we employ vectorization and 3 loops (instead of 6) in our code; this makes our code approximately 20 times faster than the schemes involving 6 loops. We mention about this in the begining of section titled "Design of the code".

shashwat-bhattacharya commented 4 years ago

And changes I suggested in #2185 (comment) are still relevant. At least processors should be changed to cores.

Thanks for the suggestions. We have done the necessary modifications.

shashwat-bhattacharya commented 4 years ago

@jedbrown does performance evaluation also include scalability? Test programs included in repository are only 2D so it might be difficult to evaluate scalability claims made in article. I suggested adding 3D test program in ShubhadeepSadhukhan1993/fastSF#2

Based on the suggestions of @iljah and @jedbrown , we have added 3D test cases (velocity and scalar structure functions). Please refer to README.md for details.

Thanks for the suggestions.

shashwat-bhattacharya commented 4 years ago

@jedbrown @iljah @cpgr We thank you for a comprehensive review of our code. We have implemented your suggestions to the best of our ability. We will welcome further comments, if any. We hope that our work is now suitable for publication in JOSS.

jedbrown commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

iljah commented 4 years ago

Thanks @shashwat-bhattacharya for the changes. @jedbrown to me this looks good to go.

jedbrown commented 4 years ago

Thanks for your reviews and my apologies for my greatly delayed reply. I made some copy edits (see PR) and also encountered some further issues that I'd like you to consider.

It looks like any MPI-1 implementation should be sufficient. I.e., one need not require MPICH.
Software dependencies should be cited when possible. For example, this appears to be the preferred citation for HDF5. In a JOSS paper, it's better to explain the use of the software by name, as in "fastSF uses MPI [cite] for parallelism, HDF5 [cite] via H5SI [cite] for reading gridded field data and writing structure functions, as well as blitz++ [cite] for vectorized computation and yaml-cpp [cite] for reading control parameters."
It should be possible to use pkg-config or other compile-time configuration to use dependencies from their existing locations. It's poor packaging practice to require dependencies to be in specific directories.
If there is a memory requirement that each process must be able to redundantly store the entire grid, it should be stated clearly. This becomes an issue if one runs around 1000^3. (Have you considered using MPI shared memory constructs so that processes on a node can share the same read-only data?)
I think it would be really useful for the application to take its configuration via the command line. This is much more convenient for scripting, in which one might have a loop around calling fastSF. You could keep the YAML files for defaults, but I think it's poor practice to not allow the user to configure the task via the command line.
The input and output files should also be specifiable at run-time.
Is there a reason why the grid dimensions must be specified up-front instead of being read from the HDF5 files?
It's usually best practice to write a single HDF5 file with multiple fields instead of one file per field. Would support for such schema be hard to add?
The supported HDF5 schema should be documented prominently (e.g., in the README). There should be error handling to report what is wrong if a users file does not conform to the schema. If double precision is required, this should also be documented.
It appears that CMake is only required to build h5si, but not for fastSF. As such, it should not be named as a dependency.
We usually ask that Python software be pip installable. The nearest thing for C++ software in the HPC space would be adding it to Spack. This is not required, but does significantly increase likelihood of use and the process tends to fix portability quirks.
APA style says that journal names should not be abbreviated.

shashwat-bhattacharya commented 4 years ago

Thanks @jedbrown for your suggestions. We will implement them and get back to you soon.

shashwat-bhattacharya commented 4 years ago

@jedbrown I would like to thank you again for your useful recommendations. We have implemented them to the best of our ability as follows:

We mention in the README and in the revised paper that any MPI-1 implementation will suffice.
We have cited all the software dependencies in the revised paper.
In the updated code, it is possible to use pkg-config to use yaml-cpp and blitz++ libraries. However, we could not find pkg-config files for hdf5 and h5si libraries.
We have added a section on estimating the memory requirement in the README file. We agree that making all the processors store the entire grid makes the code very memory intensive. However, due to the nature of our algorithm, storing the grid in one master processor will require complex communications between the processers which can slow down the code. Moreover, computing the structure functions on very large grids (above 512^3) is not really required because structure functions are generally important in the intermediate scales. Thus, one can coarsen the input data to 512^3 grid or lower resolutions and still get accurate results for the structure functions with less memory requirement.
We have implemented the facility of accepting inputs via command line. If the user does not specify a particular parameter via command line, then that parameter value is taken from para.yaml file. It should now be more convenient for the user for scripting.
The input and the output names are now specifiable at run-time. If these names are not passed, then the default naming convention (as it was before) will be followed.
The grid dimensions are read from the parameters file because we first resize the field array before feeding the input data to it from the hdf5 file. Not doing so can cause segmentation faults. This seems to be a minor drawback of either h5si or Blitz++ library.
The revised code stores the output in two hdf5 files for velocity structure functions (one for longitudinal and the other for transverse) and one hdf5 file for scalar structure functions. Each hdf5 file has multiple datasets corresponding to different orders of the structure function. Thus, each dataset stores the structure functions of a particular order. Please note that in the earlier code, different orders of structure functions were stored in separate hdf5 files. Thus, the output file schema is more compact in the revised code.
The revised README file has clear instructions on the supported HDF5 schema. Please note that currently the code accepts only double-precision values. If there is an error in the user's schema, the code will display a checklist for the schema.
CMake has been removed from the list of dependencies.
The idea of Spack looks good, and we are planning to add fastSF to Spack for later versions. We thank you for this suggestion.
The journal names are written in full in the bibliography of the revised paper.

We thank you again for these useful suggestions. They have definitely improved the code. I hope the paper is now suitable for publication. We will welcome further comments, if any.

labarba commented 4 years ago

hi 👋 – @jedbrown, it looks like the author is waiting for your assessment of their mods. Pop in and have a look!

ShubhadeepSadhukhan1993 commented 4 years ago

Hello @jedbrown, We have implemented your suggestions to the best of our ability. We are waiting for further course of action. Any further suggestions from you will be welcome. Thanking you.

shashwat-bhattacharya commented 4 years ago

Hi @jedbrown , we are awaiting your response. We are using this code for our subsequent work and thus it will be helpful if this paper is published soon. Please let us know if anything further needs to be done to the code or the manuscript such that it is suitable for publication in JOSS.

Kevin-Mattheus-Moerman commented 4 years ago

@jedbrown :point_up:

jedbrown commented 4 years ago

Thanks for your updates and sorry about my delinquency getting to this.

Package managers often install pkg-config for HDF5 (e.g., Debian's libhdf5-dev). The repository has a stub but you're correct that it's often not installed. In the old days, we all used ./configure --with-hdf5-dir=/path/to/hdf5 or if using a raw Makefile, make HDF5_DIR=/path/to/hdf5 and have the makefile calculate appropriate compile and link flags. I understand you have a single-file project, but asking users to edit your makefile is clumsy and avoidable if you just use variables.
HDF5-1.8 has reach end-of-life. The last release was 1.8.21 in June 2018 "HDF5 1.8 will not be supported after the May 2020 release". I think new software should use supported versions of dependencies unless there is a really strong reason not to (e.g., license incompatibility).
HDF5 has solid support for reading metadata (including shapes) before reading the array data. It seems like h5si has a shape() method that may do what you need: https://github.com/anandogc/h5si/blob/master/trunk/lib/h5dataset.h#L104 (There don't seem to be any docs or nontrivial tests so who knows if it works.)
It's good practice for command-line executables to respond to -h and/or --help to document their options.
Why do the input files need to have special names (according to the README)? Why not just scan one or more input files looking for datasets with the specified names? It just seems like an odd redundancy that causes friction compared to how everyone else creates HDF5 files.

IMPORTANT: Please note the following:

The input files can have different names than the ones specified above, but it needs to be ensured that the hdf5 file name and the dataset name are the same (except that the dataset name does not have .h5 extension. The input file names need to be specified via command-line arguments (explained later).

@whedon generate pdf

openjournals / joss-reviews