-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
[REVIEW]: TBFMM: A C++ generic and parallel fast multipole method library #2444
Comments
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @pitsianis, @Himscipy, @sarats it looks like you're currently assigned to review this paper 🎉. 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. ⭐ Important ⭐ 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:
For a list of things I can do to help you, just type:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
|
I have been unable to edit/complete checklist above. Whedon has been forgetful and didn't assign me. |
The reviewer already has a pending invite. @sarats please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations |
Actually, you have to accept the invitation. See the instructions in the first comment in this issue. |
Done. Thought I did this only once when I first reviewed for JOSS. Didn't realize it's required for every review. |
@berenger-eu : Based on 👉 Check article proof 📄 👈 Here are some of the comments regarding the Software Paper, for you to take them into account.
Section wise comments..
Thank you. |
@berenger-eu; Following are my comment regarding the documentation for the library. Overall Comment: The Readme of the code is said to be the main documentation for the code. The Readme is nicely done with details including the installation/compilation instruction ,usage and customization instructions. But navigating a large README for the purpose of usage is challenging. Therefore, I have two options for the author (op-1) Create a Table of Content in the Readme file (low-effort) (op-2) Use the instruction provided at the github-wiki, making pages to create a easy to navigate doc.(medium-effort; hight impact), I would recommend option-2. Another suggestion, would be to define separate wiki-pages for titles such "Cell", "Particle", "Trees", "Algo" etc. for organization purposes.
|
Dear Himanshu Sharma (@Himscipy) Here is my answer.
Thanks, this has been changed.
The new version of the paper includes results with OpenMP. As it is described in the related section, there is a speedup with OpenMP 4 but the scalability is limited (because the mutexinout/commutative access is only supported from version 5).
TBFMM will be executed on ARM CPU soon. But we definitively need to add MPI in order to run on multiple nodes, and this will be our next feature.
I updated the sentence and tried to check the grammar.
I updated the sentence and tried to check the grammar.
I updated the sentence and tried to check the grammar.
I fully agree that both were needed. I added a TOC in the README, and I add a copy in the wiki: https://gitlab.inria.fr/bramas/tbfmm/-/wikis/home
I added sentences from the paper.
I renamed the sections.
Noted.
I feel that adding the TOC solved the problem, I look forward to your confirmation.
Noted.
Noted. Remark It seems that an invisible bad char was in the paper.md (and the README.md) such that the section names were not right (for instance, the ## Features section was written in plain text in the paper). Therefore, I hope that rebuilding the paper will fix this. @whedon generate pdf |
@whedon generate pdf |
@berenger-eu Thank you for addressing the documentation comments and for the response. Here are my comments about the installation process/ functionality testing;
I have couldn't raise issue with the build log file, I cannot create issue on the repo, since it was asking some INRIA login access. I have added the build log below for your reference. I see that the issue could be my compiler but please let me know and if you can provide some additional comment or procedure to build it.
ProductName: Mac OS X Build Error Below
|
Dear @Himscipy, Thanks for your message.
You are right that this was definitively missing.
Unfortunately, the Inria gitlab access policy changed few weeks ago.
Surprisingly there are some errors with Mac + clang (but it works with Linux + clang or even Mac + gcc). Regards. |
Hi @berenger-eu , Thank you for the reply and resolving the issue. I am able to compile the package. Regards, |
👋 @berenger-eu, Thank you for fixing the issue I was able to use the library and the functionality. Although I don't find the TOC in the git repo, which I am able to see on the gitlab.inria.fr so please check. Thank you @danielskatz for providing the opportunity to review the work, I am done with my review. Regards, |
Dear @pitsianis @sarats, I just would like to know if I missed something and if the ball is on my side or not. If that is the case, please could you point me to what I am supposed to do? Otherwise, I will simply wait for comments. Thanks a lot. |
@berenger-eu Himanshu has done a comprehensive review and got to some of the documentation issues I ran into. Just a couple of general questions/comments about computational approach:
|
A minor comment: I have noticed related work at Supercomputing conference series over the years and your related work/references didn't appear to capture those. So, adding some discussion on how your approach differs from those would be great. |
@whedon generate pdf |
Dear @sarats thanks for your comments.
This is our next objective. However, the FMM has an irregular data structure (the tree) and we would like to stay at a high-level (C++ and not C). Consequently, most runtime systems are not adapted. Therefore, we plan first to improve our SPETABARU runtime system to support GPUs (this is a wok in progress) and second to validate this feature on TB-FMM.
I did a small test and it crashes with Intel compiler. I did a small investigation, and it seems that Intel compiler is doing something wrong. If I leave the tasks but disable them it crashes (by adding "if(0)" at the end of the pragma => so no tasks but synchronous execution with all the Intel OpenMP machinery). If I simply comment the "#pragma" (so no tasks and no OpenMP machinery) it works. So I conclude that the Intel OpenMP has a bug, which certainly comes from the use of C++ in my opinion. Therefore, I will add in the README that Intel compiler is currently not supported.
Both implementations are very similar, they have the same: number of tasks, tasks and priorities of the tasks. The two differences are the type of data accesses and how the runtime systems are implemented (the potential overhead coming from the tasks/dependencies management of the runtime system). But GNU OpenMP implementation is usually quite good in this aspect. Therefore, the only difference comes from the non-commutative access with OpenMP.
Yes, I know the works from G. Biros or also R. Yokota, which are great. The main difference is that we use the task-based parallelization (as we did for ScalFMM). The advantages of task-based are to avoid synchronizations, delegate load-balancing to the scheduler, and thus should be - in theory - more generic against the hardware or the underlying computation kernel. On the other hand, the gap to create a large-scale application that could run on thousands of nodes is huge and depends on the capability of the runtime system (see https://ieeexplore.ieee.org/abstract/document/8226789 for more easy work on regular linear algebra method). Whereas, classical parallelization schemes (OpenMP/CUDA with manul transfers/MPI with manual communications) has proven its potential. Therefore, I will add a single sentence to point to these references. You can see the commits that changed the paper here: Best regards. |
Hi Nikos (@pitsianis) 👋 It's been a few weeks since the other two reviewers completed their reviews and it looks like you have been busy with other work. Would you mind circling back to let the author know if there are any desired changes so that we can procede? With Thanks, |
Dear @pitsianis |
@whedon generate pdf |
@whedon check references |
|
Hi @berenger-eu (:wave:) : In addition to fixing the above two missing DOIs flagged by whedon, I recommend the following minor grammatical improvements to the article: "In addition, the algorithm also requires that the kernel to approximate far interaction exists as providing an approximation kernel for a physical equation can be challenging" => I recommend making the following a separate sentence: ", however, when the algorithmneeds cells outside of the boundaries, it selects cells at the opposite side of the simulation box." "This method needs nothing more than a FMM kernel," => "We remind that OpenMP 4.5 does not support mutexinout, the commutative data access." => "In our case, the resulting degree of parallelism is then too limited to feed all the cores available" => I recommend making the following phrase a separate sentence rather than being separated with a comma: ", instead, we will wait that the OpenMP library implementations supportmutexinout" "test cases: two simulations of one and ten millions of particles randomly distributed in a cube." => |
@whedon check references |
|
@whedon generate pdf |
Dear @poulson Thanks a lot for the comments! |
Thank you, @berenger-eu! Would you mind creating a DOI on Zenodo and sharing it here? |
@poulson Yes, here it is https://doi.org/10.5281/zenodo.4302384 |
@whedon set 10.5281/zenodo.4302384 as archive |
OK. 10.5281/zenodo.4302384 is the archive. |
@whedon accept |
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉 openjournals/joss-papers#1954 If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#1954, then you can now move forward with accepting the submission by compiling again with the flag
|
I had a quick look, seems OK. |
@whedon accept deposit=true |
|
🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦 |
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨 Here's what you must now do:
Any issues? Notify your editorial technical team... |
Congrats @berenger-eu on your article's publication in JOSS! Many thanks to @pitsianis, @Himscipy, and @sarats for reviewing this, and @poulson for editing. |
🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉 If you would like to include a link to your paper from your README use the following code snippets:
This is how it will look in your documentation: We need your help! Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:
|
Submitting author: @berenger-eu (Bérenger Bramas)
Repository: https://gitlab.inria.fr/bramas/tbfmm
Version: v1.0
Editor: @poulson
Reviewer: @pitsianis, @Himscipy, @sarats
Archive: 10.5281/zenodo.4302384
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:
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
@pitsianis & @Himscipy & @sarats, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @poulson know.
✨ Please try and complete your review in the next six weeks ✨
Review checklist for @pitsianis
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @Himscipy
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @sarats
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
The text was updated successfully, but these errors were encountered: