[REVIEW]: cuIBM: A GPU-based immersed boundary method code

whedon commented 7 years ago

Submitting author: @mesnardo (Olivier Mesnard) Repository: https://github.com/barbagroup/cuIBM Version: v0.1 Editor: @kyleniemeyer Reviewer: @arghdos Archive: 10.5281/zenodo.832338

Status

Status badge code:

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

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) 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 questions

@arghdos, please carry out your review in this issue by updating the checklist below (please make sure you're logged in to GitHub). The reviewer guidelines are available here: http://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @kyleniemeyer know.

Conflict of interest

[x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (such as being a major contributor to the software).

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] Version: Does the release version given match the GitHub release (v0.1)?
[x] Authorship: Has the submitting author (@mesnardo) made major contributions to the software?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: Have any performance claims of the software been confirmed?

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 function 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] Authors: Does the paper.md file include a list of authors with their affiliations?
[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] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

whedon commented 7 years ago

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks for JOSS. @arghdos it looks like you're currently assigned as the reviewer for this paper :tada:.

: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 as reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all JOSS 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

kyleniemeyer commented 7 years ago

@arghdos and @osumageed, this is where the review will take place.

@arghdos since you are the "primary" reviewer (and JOSS is only really set up to handle one reviewer), I'll ask you to check off the boxes at the top. However @osumageed you should also go through and make sure you agree that they are satisfied. Thanks!

skyreflectedinmirrors commented 7 years ago

@kyleniemeyer @mesnardo

I have completed a preliminary review of cuIBM. This is a valuable software that enables CUDA based solution of the 2D incompressible Navier-Stokes equations using immersed boundary methods. Installation is relatively easy, and is laid out comprehensively in the Readme, there are many examples for use, and there is API level documentation of the doe.

Most of the requirements have been met, but a few issues remain. Two simple issues have been opened on the cuIBM repo:

#32 - Add a link to the API to the readme for easier access to users.
#33 - Add contribution guidlines, as required for JOSS acceptance (there are examples given in the link left in the issue)

However, there is an open question of on validation of performance. In one of the early papers linked to in this repo, there are claims of up to 4-8x solution speedup over a single CPU core. However, it appears the CPU implementation of cuIBM is not currently functional hence there is no way to validate the performance of cuIBM against any "reference technique". I am not sure how to proceed with this issue

OSUmageed commented 7 years ago

@kyleniemeyer @arghdos @mesnardo

I completely agree with @arghdos' preliminary review. And I have a few things to add.

First, I want to say the documentation is really spectacular. I'm still relatively new to this field, but this is the most well documented source code I've ever read.

I definitely think there needs to be a link to the API in the readme or instructions to build and view it, if only because it allows the user to copy and paste multiple lines of code into a terminal at once which the > symbol in the readme prevents.

The lack of performance validation is an issue. It doesn't need to be a full example; a study with plots and an adequate description of the hardware that produced them would suffice. The performance is essential to the statement of need. If it's not faster on the GPU, why not write it in C/C++?

I also opened an issue in the repo #34. The plotting scripts for the flying snake examples don't work.

mesnardo commented 7 years ago

Thank you @arghdos for the preliminary review and for your suggestions.

I have added a link to the Doxygen API documentation in the README of cuIBM. (Note that there is also a link to API documentation at the top of the GitHub page of cuIBM, next to the title.) I have also added community guidelines in the file CONTRIBUTING.md.

Those changes are now available in the branch joss-reviews-hotfix-0.1.2. This branch will be merged into master and a new release (v0.1.2) will be created once we have satisfied all your requests.

kyleniemeyer commented 7 years ago

Thanks for your reviews @arghdos and @OSUmageed! Do the documentation and community guideline changes that @mesnardo made satisfy those concerns? If so, you can close those issues / check the boxes above.

@mesnardo It seems like the performance claim is the only remaining issue—do you have a response on that?

skyreflectedinmirrors commented 7 years ago

@kyleniemeyer I've closed my issues and updated the review

OSUmageed commented 7 years ago

@kyleniemeyer @arghdos @mesnardo I closed my issue as well.

mesnardo commented 7 years ago

@arghdos @OSUmageed Thank you for your reviews and closing the issues.

@kyleniemeyer Concerning the performance and the fact that the CPU routines are not functional, I am waiting for the input from @anushkrish who has run the tests to compare the GPU version of cuIBM to the CPU one.

kyleniemeyer commented 7 years ago

@mesnardo ok great! That looks like the only outstanding issue.

arfon commented 7 years ago

@kyleniemeyer - is this good to accept now?

kyleniemeyer commented 7 years ago

@arfon no, still waiting on this outstanding performance issue to be resolved

labarba commented 7 years ago

@kyleniemeyer @arghdos There is no outstanding performance issue — the CPU version is not maintained, not supported, and there is a note to that effect in the README. The reviewer is referencing an old preprint from many years ago, not this paper. We make no CPU performance claims in this paper, nor on the repository of the software. The software repository only includes performance claims in the form of runtime for the examples, on GPU.

kyleniemeyer commented 7 years ago

OK, @labarba @mesnardo, as long as there is no performance claim vs CPU. So, it looks like the software itself is good to go.

However, the article itself has a few outstanding issues:

[x] The references in the paper are missing DOIs (where appropriate). Could you add those to the paper.bib file?
[x] Could you add a more general statement of need perhaps to the beginning of the article, that a broader audience would understand? For example, something about fluid-structure interaction would be appropriate, which isn't currently mentioned.
[x] The sentence beginning with "Among them" is missing a verb.

Once those things are resolved, this submission will be ready for publication.

labarba commented 7 years ago

We'll look into that, and we also just discussed with @mesnardo that we'll remove the CPU routines from the master branch and leave it in a deprecated branch.

BTW, this software does not compute (two-way) fluid-structure interaction (only prescribed body motions).

labarba commented 7 years ago

(And, as you heard me rant at SciPy, we really have to stop talking about GPU vs. CPU comparisons. This is a fixation that is an unfortunate consequence of the vendor marketing, and it no longer makes sense.)

kyleniemeyer commented 7 years ago

@labarba sounds good, thanks! And yes, I am well aware of this—hence the development of our fork of cuIBM that can do so 😀. Perhaps "fluid-structure interaction" would be misleading then, but a statement about simulating flows interacting with moving bodies (e.g.) would be helpful.

mesnardo commented 7 years ago

@kyleniemeyer , we have removed the CPU routines, there are now in a separate branch called deprecated-cpu-routines. Also, we have updated the paper and the references with the suggestions you made above.

kyleniemeyer commented 7 years ago

OK, looks good! One last thing: the Known Issues at the bottom still says "CPU routines do not work." Could you correct that, and then provide a DOI for the archived software here?

mesnardo commented 7 years ago

You should be looking at the branch joss-reviews-hotfix-0.1.2; it includes all the changes made during the peer-review. We are waiting for acceptance before making the new archive and getting a DOI.

kyleniemeyer commented 7 years ago

@mesnardo ah, got it—I was indeed looking at the master README.

OK, this is accepted for publication, so please merge that branch and archive the software.

mesnardo commented 7 years ago

@kyleniemeyer Branch joss-reviews-hotfix-0.1.2 has been merged into master, cuIBM-0.1.2 has been released and uploaded to Zenodo (10.5281/zenodo.832338).

kyleniemeyer commented 7 years ago

@whedon set 10.5281/zenodo.832338 as archive

whedon commented 7 years ago

OK. 10.5281/zenodo.832338 is the archive.

kyleniemeyer commented 7 years ago

Hi @arfon this is ready to publish!

arfon commented 7 years ago

@arghdos many thanks for your review here and @kyleniemeyer for editing this submission ✨

@mesnardo - your paper is now accepted into JOSS and your DOI is http://dx.doi.org/10.21105/joss.00301 ⚡️ 🚀 💥

kyleniemeyer commented 7 years ago

Thanks @arghdos and @OSUmageed!

mesnardo commented 7 years ago

Yay! Thank you very much @kyleniemeyer , @arghdos , and @OSUmageed for your review of cuIBM and your useful suggestions!

openjournals / joss-reviews