search

openjournals / joss-reviews

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

[REVIEW]: ExaFMM: a high-performance fast multipole method library with C++ and Python interfaces #3145

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @tingyu66 (Tingyu Wang) Repository: https://github.com/exafmm/exafmm-t Version: v0.1.1 Editor: @poulson Reviewer: @berenger-eu, @pitsianis Archive: 10.5281/zenodo.4874663

: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

Status badge code:

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

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

@berenger-eu & @pitsianis, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. 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 @poulson 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 ✨

Review checklist for @berenger-eu

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @pitsianis

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @berenger-eu, @pitsianis 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:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. 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 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.60 s (148.9 files/s, 109491.9 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
Bourne Shell                     10           5185           6020          31749
m4                               21           1108            103          10913
C/C++ Header                     20            285            649           5223
C++                              14            239            236           1325
TeX                               1             18              0            235
Markdown                          5             30              0            218
CMake                             2             49             89            209
Python                            2             67            123            130
reStructuredText                  5             70             71             98
make                              4             25             11             91
Jupyter Notebook                  3              0            760             79
YAML                              1              1              0             37
Bourne Again Shell                1              1              0              2
--------------------------------------------------------------------------------
SUM:                             89           7078           8062          50309
--------------------------------------------------------------------------------

Statistical information for the repository '7afc6c45bbafd3f855b4e2ef' was
gathered on 2021/04/02.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Rio Yokota                      29         51608           1937           22.44
Tingyu Wang                    491         67535         108103           73.61
bloodysin                       67          4335           5075            3.94

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
Rio Yokota                 1131            2.2          9.6                0.18
Tingyu Wang                6945           10.3         17.2               14.31
bloodysin                   201            4.6         28.0                4.98
whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1109/IPDPS.2008.4536319 is OK
- 10.1016/j.jcp.2011.12.024 is OK
- 10.1109/5992.814662 is OK
- 10.1016/j.jcp.2003.11.021 is OK
- 10.1002/nme.2972 is OK
- 10.1016/B978-0-12-384988-5.00009-7 is OK
- 10.6084/m9.figshare.92166.v1 is OK
- 10.1177/1094342011429952 is OK
- 10.1260/1748-3018.7.3.301 is OK
- 10.4208/cicp.020215.150515sw is OK
- 10.1145/2588768.2576787 is OK
- 10.1145/2590830 is OK
- 10.21105/joss.02879 is OK
- 10.21105/joss.02444 is OK

MISSING DOIs

- None

INVALID DOIs

- 10.1016/0021-9991 is INVALID
whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

berenger-eu commented 3 years ago

Dear @tingyu66,

I have a single tiny remark concerning the paper, which is clear and answers all the questions I had. Instead of citing "Blanchard, P., Bramas" for ScalFMM, I would suggest citing the SISC paper: https://epubs.siam.org/doi/abs/10.1137/130915662

I was able to compile the code (using the dependencies already installed on my computer) and run the tests. I looked at the CI and saw that it tests compilation by loading the same modules as the ones given in the documentation. I was able to install the python package and run an example. I looked at the source code, and it is clear and compact, as described in the paper. Furthermore, I got similar performances. From my side, the documentation is also fine.

Therefore, I have only one remark, and it concerns the authors: I saw that a fourth person (bloodysin stvgm1993@gmail.com) participate in the development. I would suggest to make sure it is fine if this person is not in the list of the authors (who is maybe an alias to one of the authors).

I have also a question that I ask because I am interested to know more, but that does not need to be addressed in the paper. I saw on the TODO.md that you plan to use GPUs. Do you already have an idea of how you will do it? Do you plan to use a runtime system, maybe legion?

Thanks.

labarba commented 3 years ago

hi @berenger-eu 👋 — first, thank you for being a reviewer! Answering your question about the author list: the user bloodysin was a master's student with Rio a while back, and even though you see a bunch of commits, this was all hacking that eventually got removed, or in other words, he did not contribute meaningfully to the final product being submitted for JOSS.

Regarding your questions on GPUs, this is not in the roadmap now, and no GPU code is being submitted for review.

whedon commented 3 years ago

:wave: @pitsianis, please update us on how your review is going (this is an automated reminder).

poulson commented 3 years ago

I'm just checking in as well. I'm not going to ping since everyone is busy and whedon is doing that automatically, but please let me know if there are any obstacles I can clear.

pitsianis commented 3 years ago

Thanks! Please renew my invitation to review, because it has expired.

poulson commented 3 years ago

Hi @pitsianis -- I'm not sure what you mean, as the review is currently active and started. Are you referring to the pre-review invitation?

pitsianis commented 3 years ago

I am not able to fill in the checkmarks

danielskatz commented 3 years ago

@whedon re-invite @pitsianis as reviewer

whedon commented 3 years ago

OK, the reviewer has been re-invited.

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

pitsianis commented 3 years ago

@tingyu66, I have not been able to compile on macos and linux. The first one makes the most progress with the call

./configure CXX=g++-10

but it fails with

checking fftw3.h usability... no
checking fftw3.h presence... no
checking for fftw3.h... no
checking for fftwf_malloc in user specified location... no
checking for fftw_malloc in user specified location... no
configure: error: cannot find fftwf library.

even though I have fftw installed with brew.

The linux attempt fails with 

checking for C++ compiler vendor... unknown
checking for OpenMP flag of C++ compiler... unknown
configure: error: don't know how to enable OpenMP for C++
 /t/exafmm-t-master> g++ --version
g++ (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
tingyu66 commented 3 years ago

Hello @pitsianis, our code currently does not support MacOS because many macros in our configure script have a hard time checking libraries on MacOS.

However, it should work on Linux though. I tested many versions of g++ ranging from 5.4.x to 10.2.x. It seems that the configure script did not find your g++ as the C++ compiler. Did you use ./configure CXX=g++ on Linux?

pitsianis commented 3 years ago

On Ubuntu 18.04.5 LTS (GNU/Linux 4.15.0-118-generic x86_64) the ./configure CXX=g++ stops with

checking for fftw3.h... yes
checking for fftwf_malloc in user specified location... no
checking for fftw_malloc in user specified location... no
configure: error: cannot find fftwf library.
tingyu66 commented 3 years ago

On Ubuntu 18.04.5 LTS (GNU/Linux 4.15.0-118-generic x86_64) the ./configure CXX=g++ stops with

checking for fftw3.h... yes
checking for fftwf_malloc in user specified location... no
checking for fftw_malloc in user specified location... no
configure: error: cannot find fftwf library.

Exafmm-t requires the FFTW3 shared library in both single and double precision (libfftw3f.so and libfftw3.so), so we suggest to install libfftw3-dev package, as described in our documentation:

apt-get update
apt-get -y install libfftw3-dev

If you want to build FFTW3 library from source, you probably need to compile separately for each precision if I remember correctly. Then add the directory of the shared libraries to LIBRARY_PATH and LD_LIBRARY_PATH environment variables.

pitsianis commented 3 years ago

I am done with review. I like the Jupyter notebook.

poulson commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

poulson commented 3 years ago

@whedon check references

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1109/IPDPS.2008.4536319 is OK
- 10.1016/j.jcp.2011.12.024 is OK
- 10.1109/5992.814662 is OK
- 10.1016/j.jcp.2003.11.021 is OK
- 10.1002/nme.2972 is OK
- 10.1016/B978-0-12-384988-5.00009-7 is OK
- 10.6084/m9.figshare.92166.v1 is OK
- 10.1177/1094342011429952 is OK
- 10.1260/1748-3018.7.3.301 is OK
- 10.4208/cicp.020215.150515sw is OK
- 10.1145/2588768.2576787 is OK
- 10.1145/2590830 is OK
- 10.21105/joss.02879 is OK
- 10.21105/joss.02444 is OK

MISSING DOIs

- None

INVALID DOIs

- 10.1016/0021-9991 is INVALID
tingyu66 commented 3 years ago

Hi @poulson, I just fixed the invalid DOIs and updated one reference as suggested by @berenger-eu in the master branch.

poulson commented 3 years ago

Thanks -- I noticed that the paper could use a round of edits and that is what has been delaying me. I am going to rebuild and respond with a round of suggestions shortly.

poulson commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

poulson commented 3 years ago

Here is a first batch of suggestions (all before the 'Statement of Need' section):

I recommend changing

  1. "It is re-written with a new design, after multiple re-implementations of the same algorithm, over a decade of work in the research group." to "Our current implementation is the most recent of many across a decade of work in our research group."

  2. "for what is an intricate and difficult algorithm to implement" to "for an intricate algorithm"

  3. "The Python binding in this new version allows calling it from a Jupyter notebook, reducing barriers for adoption. It is also easy to extend, and faster or competitive with state-of-the art alternatives." to "Our new header-only C/C++ implementation is easier to extend, still competitive with state-of-the-art implementations, and includes a pybind11 Python interface."

  4. I recommend deleting the sentence "Together with the likes of Krylov iterative methods and the fast Fourier transform, FMM is considered one of the top 10 algorithms of the 20-th century."

  5. I recommend combining the surrounding two sentences of (4) into "The fast multipole method (FMM) was introduced more than 30 years ago as a means of reducing the complexity of N-body problems from O(N^2) to O(N) using hierarchical approximations of long-range interactions." There is no need for the parenthetical explanation of N since that is suggested by the "N-body" problem reference.

  6. I recommend tweaking the statement "Two variants of hierarchical N-body algorithms exist" to "two major variants", and perhaps even mentioning that structures matrix algorithms (such as H-matrices, etc.) are algebraic analogues of FMM and tree codes.

  7. I recommend rewording the later sentence to "For example, the integral formulation of problems modeled by elliptic partial differential equations can be reinterpreted as N-body interactions."

  8. "kernel-independent variant of the algorithm" to "kernel-independent variant of FMM"

  9. We can now delete the sentence "ExaFMM is a header-only library written in C/C++ and provides a Python API using pybind11" due to its incorporation above.

poulson commented 3 years ago

A second round of suggested edits:

  1. Move "The first version of ExaFMM focused on low-accuracy optimizations and implemented a dual-tree traversal. It was GPU-enabled using CUDA, distributed via MPI, and multi-threaded via OpenMP." into separate paragraph alongside "Despite these efforts, it has remained a challenge in the FMM community to have a well-established open-source software package analogous to FFTW for the fast Fourier transform."

  2. Paragraph break at "The 'alpha' version of ExaFMM...poor reusability and maint ainability." [Delete "as is the case with various other codes in this field. Already it was the fourth implementation of the algorithm within our research group, and two more came about over the years before the version we present in this paper." as the former is unfair without citations and the latter is redundant with earlier discourse.]

  3. Further delete the following section due to it being more appropriate for a blog post or extended footnote: "As a bit of history of this project...and to increased scientific impact."

labarba commented 3 years ago

@poulson — I made all the suggested edits, though I'm sad of deleting the historical paragraph (commit 1a9146f). It's a road paved with tears.

poulson commented 3 years ago

A long footnote might make sense as well! I just think it is a lengthy aside that distracts from the conciseness that is typical of JOSS articles.

poulson commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

poulson commented 3 years ago

@labarba Please let me know when you are comfortable with the edit and, after sharing the preferred DOI, I can set it and mark as ready for publication.

labarba commented 3 years ago

A long footnote might make sense as well! I just think it is a lengthy aside that distracts from the conciseness that is typical of JOSS articles.

That's OK. We'll find another place and time to document the story. (It wouldn't work as a footnote, as it had citations.)

labarba commented 3 years ago

tagged version: v0.1.1

Zenodo deposit: 10.5281/zenodo.4874663 https://doi.org/10.5281/zenodo.4874662

poulson commented 3 years ago

Thank you for the painful edit -- and apologies for it not working out as a footnote.

@whedon set 10.5281/zenodo.4874663 as doi

poulson commented 3 years ago

@whedon set 10.5281/zenodo.4874663 as doi

whedon commented 3 years ago

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

@whedon commands
poulson commented 3 years ago

@whedon set 10.5281/zenodo.4874663 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.4874663 is the archive.

poulson commented 3 years ago

@whedon recommend-accept

whedon commented 3 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1109/IPDPS.2008.4536319 is OK
- 10.1016/j.jcp.2011.12.024 is OK
- 10.1109/5992.814662 is OK
- 10.1016/j.jcp.2003.11.021 is OK
- 10.1016/0021-9991(87)90140-9 is OK
- 10.1002/nme.2972 is OK
- 10.1016/B978-0-12-384988-5.00009-7 is OK
- 10.6084/m9.figshare.92166.v1 is OK
- 10.1177/1094342011429952 is OK
- 10.1260/1748-3018.7.3.301 is OK
- 10.4208/cicp.020215.150515sw is OK
- 10.1145/2588768.2576787 is OK
- 10.1145/2590830 is OK
- 10.21105/joss.02879 is OK
- 10.21105/joss.02444 is OK
- 10.1137/130915662 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago

:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/2348

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/2348, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
labarba commented 3 years ago

I have read through the proof, and approve it.

arfon commented 3 years ago

@whedon accept deposit=true

whedon commented 3 years ago 
Doing it live! Attempting automated processing of paper acceptance...
whedon commented 3 years ago

🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦

whedon commented 3 years ago

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited :point_right: https://github.com/openjournals/joss-papers/pull/2349
  2. Wait a couple of minutes, then verify that the paper DOI resolves https://doi.org/10.21105/joss.03145
  3. If everything looks good, then close this review issue.

  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

    Any issues? Notify your editorial technical team...

arfon commented 3 years ago

@berenger-eu, @pitsianis – many thanks for your reviews here and to @poulson for editing this submission! JOSS relies upon the volunteer effort of people like you and we simply wouldn't be able to do this without you ✨

@tingyu66, @labarba – your paper is now accepted and published in JOSS :zap::rocket::boom: