openjournals / joss-reviews

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

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

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @ShubhadeepSadhukhan1993 (Shubhadeep Sadhukhan) Repository: https://github.com/ShubhadeepSadhukhan1993/fastSF Version: v1.0.0 Editor: @jedbrown Reviewers: @cpgr, @iljah Managing EiC: Kristen Thyng

Author instructions

Thanks for submitting your paper to JOSS @ShubhadeepSadhukhan1993. Currently, there isn't an JOSS editor assigned to your paper.

The author's suggestion for the handling editor is @jedbrown.

@ShubhadeepSadhukhan1993 if you have any suggestions for potential reviewers then please mention them here in this thread (without tagging them with an @). In addition, this list of people have already agreed to review for JOSS and may be suitable for this submission (please start at the bottom of the list).

Editor instructions

The JOSS submission bot @whedon is here to help you find and assign reviewers and start the main review. To find out what @whedon can do for you type:

@whedon commands
whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks.

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
Software report (experimental):

github.com/AlDanial/cloc v 1.84  T=0.12 s (755.0 files/s, 87899.4 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
HTML                            49            177            314           4172
C++                              1            284            700           1484
CSS                              3            277             44           1376
JavaScript                      31             80             66            967
TeX                              1             22              0            182
Markdown                         2            133              0            175
Python                           1             43             54            100
YAML                             3             33             51             57
Bourne Shell                     2              9             82             16
make                             1              1             43              2
-------------------------------------------------------------------------------
SUM:                            94           1059           1354           8531
-------------------------------------------------------------------------------

Statistical information for the repository '2086' was gathered on 2020/02/08.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Shashwat Bhattachary             9          1430           3323           43.78
Shubhadeep Sadhukhan             1          5799              0           53.41
shashwat-bhattachary             2           113            192            2.81

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Shashwat Bhattachary       1365           95.5          0.0                9.08
ShubhadeepSadhukhan1       2350          100.0          0.0               24.98
shashwat-bhattachary        112           99.1          0.2               99.11
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:

kthyng commented 4 years ago

@whedon invite @jedbrown as editor

whedon commented 4 years ago

@jedbrown has been invited to edit this submission.

kthyng commented 4 years ago

@jedbrown You were suggested as editor and seem like a good fit indeed.

jedbrown commented 4 years ago

Thanks for your submission. In reading the paper and checking the code, I'm concerned that the implementation strategy here is highly suboptimal, and should really be improved if it is to be widely used. At its core, the straightforward use of six nested loops to compute differences between values at every pair of points in a 3D grid is similar to that of using naive loops for matrix-matrix product in terms of having very poor cache reuse and vectorization. My calculations suggest your test on Shaheen II ran at around 5% of peak, but this operation should be floating point limited, and a high fraction of peak should be attainable with tiling and better vectorization (you're asking the compiler to do quite a lot with how the code is written at present). I also think it would be highly preferable if your parallel implementation did not perform independent IO and full storage of global data on each node. I understand this may not be a bottleneck now because everything else is so slow. I'm also looking for a value-add because a user writing the naive nested loops and handful of lines inside is likely easier than conforming to your IO standard and using your software. I would also expect there are fast approximate methods that run much faster -- do you know if such methods exist in the literature?

Also, please note that your software has a hard dependency on H5SI, which I can't find anywhere except in the tarball of unknown provenance on the non-https website. That tarball contains no license file, but has GPL comments in source files. GPL is not compatible with the BSD license in your project. We cannot evaluate your project further with this license incompatibility.

danielskatz commented 4 years ago

đź‘‹ @openjournals/dev - something happened in the initial comment in this issue where the author's github handle (I think ShubhadeepSadhukhan1993) didn't get added correctly. I could just edit the comment but I would prefer you to check the database to make sure this is correct internally.

danielskatz commented 4 years ago

đź‘‹ @ShubhadeepSadhukhan1993 - we are waiting for your response to the issues brought up by @jedbrown

shashwat-bhattacharya commented 4 years ago

@jedbrown We thank you for your comments and feedback. We are modifying the code as per your recommendations. Further, the authors of H5SI are in the process of implementing BSD license for H5SI software. We will get back to you once these modifications are done.

danielskatz commented 4 years ago

I'm going to mark this as paused for now

shashwat-bhattacharya commented 4 years ago

Hello @jedbrown We thank you for your valuable suggestions to improve the code. We have made major changes to the code to make it faster and more efficient. The following are the changes.

1) As per your suggestion, we have implemented vectorization and reduced the number of nested loops in our code. The new scheme uses only three nested loops instead of six for 3D grids and two nested loops instead of four for 2D grids. This enhances the performance of our code by a factor of 20 times or so compared to the earlier scheme. Please refer to the paper for a detailed description of the new algorithm. We believe that this algorithm is a value-added feature because typical structure function computations in literature employ schemes similar to the one used in our earlier code.

2) In the current algorithm, a straightforward distribution of tasks among the processors along x and y directions will result in a load imbalance. Therefore, we implement an algorithm to distribute the computational load equally among the processors. Please refer to the paper and the code for details of the load-distribution scheme. Our code is scalable over many processors due to equal load distribution.

3) The earlier code employed a combination of OpenMP and MPI parallelization for computing the structure functions. However, the algorithm for equal load distribution discussed above is very complex to implement using OpenMP. Therefore, we have removed OpenMP in the new code and employ MPI parallelization along both x and y (or z) directions.

4) In the new code, the computed structure function arrays are stored only in the master node. However, if the input fields are stored in one processor, we will require a lot of inter-processor communication for computing the velocity or scalar difference. Thus, all the processors read the input data in our code, saving communication time. We believe that sharing the input data in all the processors but storing the output in one processor is the most optimal configuration for our case.

5) The library “H5SI” now contains a BSD license and is available in Github. The link is given below: https://github.com/anandogc/h5si Our code currently uses this version of H5SI.

6) Finally, we have renamed our code to “fastSF”. Further, the title of our paper has been modified to “fastSF: A parallel code for computing the structure functions of turbulence”.

We thank you again for your constructive comments and hope that the code will be widely used in the turbulence community.

shashwat-bhattacharya commented 4 years ago

@danielskatz We have made changes to the code as per the suggestions of @jedbrown . A summary of these changes is given in the previous comment. Please note that the name of the code has changed to "fastSF", and the modified title is "fastSF: A parallel code for computing the structure functions of turbulence". The modified code and the paper have been pushed to the GitHub repository. Could you please restart the review process? Thank you.

danielskatz commented 4 years ago

@whedon invite @jedbrown as editor cc @openjournals/jose-eics

whedon commented 4 years ago

@jedbrown has been invited to edit this submission.

danielskatz commented 4 years ago

đź‘‹ @openjournals/dev - reminder - something happened in the initial comment in this issue where the author's github handle (I think ShubhadeepSadhukhan1993) didn't get added correctly. I could just edit the comment but I would prefer you to check the database to make sure this is correct internally.

jedbrown commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

danielskatz commented 4 years ago

đź‘‹ @jedbrown - thanks for considering this - I asked you because of the author's initial request for you, and your involvement so far, but if you feel like you are overloaded with other JOSS submissions, we can move it to someone else. Any suggestions you have for the next best editor would be welcome in this case.

jedbrown commented 4 years ago

@openjournals/dev :wave: While you're here, we'll also need the repository URL updated. (I edited in the comment, but that doesn't show up in Whedon's PDF.)

@danielskatz I always enjoy seeing @labarba at fluid dynamics workshops :-). I can take this so long as folks can accept a bit slower responses from me.

arfon commented 4 years ago

@openjournals/dev đź‘‹ While you're here, we'll also need the repository URL updated. (I edited in the comment, but that doesn't show up in Whedon's PDF.)

Sorry for the slow response. I somehow managed to unsubscribe from the @openjournals/dev notifications.

I've made both of the requested changes now to the author GitHub handle and the repository URL.

arfon commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

Kevin-Mattheus-Moerman commented 4 years ago

@whedon assign @jedbrown as editor

whedon commented 4 years ago

OK, the editor is @jedbrown

arfon commented 4 years ago

Dear authors and reviewers

We wanted to notify you that in light of the current COVID-19 pandemic, JOSS has decided to suspend submission of new manuscripts and to handle existing manuscripts (such as this one) on a "best efforts basis". We understand that you may need to attend to more pressing issues than completing a review or updating a repository in response to a review. If this is the case, a quick note indicating that you need to put a "pause" on your involvement with a review would be appreciated but is not required.

Thanks in advance for your understanding.

Arfon Smith, Editor in Chief, on behalf of the JOSS editorial team.

arfon commented 4 years ago

:wave: @jedbrown - just checking that this is in good shape to start looking for reviewers?

@ShubhadeepSadhukhan1993 if you have any suggestions for potential reviewers then please mention them here in this thread (without tagging them with an @). In addition, this list of people have already agreed to review for JOSS and may be suitable for this submission (please start at the bottom of the list).

ShubhadeepSadhukhan1993 commented 4 years ago

@arfon Thanks for the editorial process. We have chosen five names from the given list. Our suggestion list for reviewers is here.

iljah pragyansmita jmansour corentine-dev cpgr

arfon commented 4 years ago

:wave: @cpgr, @jmansour, and @iljah - would any of you be willing to review this submission for JOSS? We carry out our checklist-driven reviews here in GitHub issues and follow these guidelines: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html

cpgr commented 4 years ago

Yep, I can do this

arfon commented 4 years ago

Thanks @cpgr! I'll add you as a reviewer now but we'll need to wait for a second reviewer to accept before moving forward with the actual review.

arfon commented 4 years ago

@whedon add @cpgr as reviewer

whedon commented 4 years ago

OK, @cpgr is now a reviewer

iljah commented 4 years ago

@arfon I can review

arfon commented 4 years ago

@whedon add @iljah as reviewer

whedon commented 4 years ago

OK, @iljah is now a reviewer

arfon commented 4 years ago

@whedon start review

whedon commented 4 years ago

OK, I've started the review over in https://github.com/openjournals/joss-reviews/issues/2185.

arfon commented 4 years ago

@cpgr, @iljah - thanks so much for agreeing to review this submission. See you over in #2185 where the actual review will take place.