[REVIEW]: PetIBM: toolbox and applications of the immersed-boundary method on distributed-memory architectures

whedon commented 6 years ago

Submitting author: @labarba (Lorena A. Barba) Repository: https://github.com/barbagroup/PetIBM Version: v0.3 Editor: @kyleniemeyer Reviewer: @jedbrown Archive: 10.5281/zenodo.1255132

Status

Status badge code:

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

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 instructions & questions

@jedbrown, 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.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @kyleniemeyer know.

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

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.3)?
[x] Authorship: Has the submitting author (@labarba) 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).
[ ] 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 6 years ago

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @jedbrown 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 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

whedon commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

https://github.com/openjournals/joss-papers/blob/joss.00558/joss.00558/10.21105.joss.00558.pdf

kyleniemeyer commented 6 years ago

Hi @jedbrown just wanted to check in on the status of your review. Thanks!

jedbrown commented 6 years ago

ETA end of this week.

kyleniemeyer commented 6 years ago

@jedbrown have you been able to make progress on this review?

labarba commented 6 years ago

Hi @jedbrown -- @mesnardo and @piyueh have been working on the joss-review branch, addressing the issues you opened on the code repo itself.

jedbrown commented 6 years ago

A couple final comments after iterating in the PetIBM repository.

I don't know why the 2D simulation is run via petibm-tairacolonius while the 3D uses petibm-decoupledibpm. Do these need to be separate executables and are these the best names for a user?
Authorship The corresponding author (@labarba) is not the author or committer of any code for this project, but has advised the authors. I imagine this counts for JOSS, but it may be helpful to rephrase "made major contributions to the software?"
It would be a lot more convenient (given vis user interfaces) if there was a single Xdmf file containing all the variables instead of separate files for each variable.
The package installs headers and a library (shared and static, plus libtool), suggesting that calling the library (rather than running the provided executables) is an intended mode of operation. However, there is no documentation of this, instead focusing exclusively on the provided executables. If the libraries are not intended to be called by user code, I would recommend not installing the headers and only installing the used library libpetibm.so.0. If it is intended for use, then it should be documented and the library should be cleaned of non-namespaced symbols (e.g., readSingleYAML).

labarba commented 6 years ago

Contributions are not always commits. Discussions on design, code reviews, and project management are important contributions to software that may not reflect on git commits. JOSS recognizes this and it's up to the authors to determine.

jedbrown commented 6 years ago

@labarba I didn't mean to suggest otherwise, but it's hard for a reviewer to audit when looking at the repository. Some journals ask for explicit statements describing the contribution from each author. In case of contributions that are not visible in the repository (commits and citations in commit messages are the most durable record), perhaps JOSS should request such a statement. (I have no doubts about your contributions so this is purely a process matter regarding the assertion the reviewer is asked to make.)

mesnardo commented 6 years ago

@jedbrown, thank you for those useful comments. Below is our reply to them.

I don't know why the 2D simulation is run via petibm-tairacolonius while the 3D uses petibm-decoupledibpm. Do these need to be separate executables and are these the best names for a user?

The executables petibm-tairacolonius and petibm-decoupledibpm both work with 2D and 3D configurations. petibm-tairacolonius solves the Navier-Stokes equations with an Immersed Boundary Projection Method (IBPM) proposed by Taira and Colonius (2007). petibm-decoupledibpm is a variant of the IBPM, proposed by Li et al. (2016), that decouples the pressure field from the Lagrangian boundary forces. Currently, there is no 3D example using the IBPM approach (petibm-tairacolonius) incorporated in the software package (folder examples/ibpm). In the folder examples/decoupledibpm, there are 2D and 3D examples using the decoupled approach (petibm-decoupledibpm).

For consistency, we have renamed the executable petibm-tairacolonius into petibm-ibpm (examples and documentation have also been update to reflect this change). We have also added more details about the program executables installed by the package to the documentation (file runpetIBM.md). (There, we now mention explicitly that those programs work for 2D and 3D configurations.)

From the perspective of using PetIBM as a library, the source code of the solvers serve as coding examples. And separating different methods into different executables makes the source code of each solver more understandable. When PetIBM's users read the source code and try to learn how to write a solver using PetIBM, it's easier for them to follow the code of a single solver at a time.

References:

Taira, K., & Colonius, T. (2007). The immersed boundary method: a projection approach. Journal of Computational Physics, 225(2), 2118-2137.
Li, R. Y., Xie, C. M., Huang, W. X., & Xu, C. X. (2016). An efficient immersed boundary projection method for flow over complex/moving boundaries. Computers & Fluids, 140, 122-135.

It would be a lot more convenient (given vis user interfaces) if there was a single Xdmf file containing all the variables instead of separate files for each variable.

We agree, it would be more convenient. With PetIBM, the Navier-Stokes equations are solved on staggered grids and we have not (yet) found a working solution to have several multiple Domain blocks in the same XDMF file that are readable by VisIt or ParaView.

The package installs headers and a library (shared and static, plus libtool), suggesting that calling the library (rather than running the provided executables) is an intended mode of operation. However, there is no documentation of this, instead focusing exclusively on the provided executables. If the libraries are not intended to be called by user code, I would recommend not installing the headers and only installing the used library libpetibm.so.0. If it is intended for use, then it should be documented and the library should be cleaned of non-namespaced symbols (e.g., readSingleYAML).

PetIBM is primarily a library (a toolbox to solve the Navier-Stokes equations with an immersed-boundary method) and we use it to develop application codes to implement different types of immersed-boundary methods. The package installs headers, a library, and executables of the immersed-boundary methods implemented. From the user perspective, the binary programs can be directly used to compute and post-process the flow using one of the already implemented immersed-boundary methods. The headers and library can be used to create new application codes.

In the examples folder, we have added a sub-folder api_examples that contains two examples of application codes using the library: a simple Navier-Stokes solver and an oscillating-cylinder example. (The Navier-Stokes example contains a README and extensive code comments to guide the user on how to use the API.) In the documentation, we have added notes regarding the usage of the PetIBM library (file usepetibmapi.md).

The non-namespaced symbols are private functions used only by other PetIBM public functions. Their prototypes are not in any headers installed. And they are not shown in the Doxygen pages.

jedbrown commented 6 years ago

Thanks for the explanation and improved documentation. I wonder if there could be more clarity about what needs to be done differently to run ibpm versus decoupledibpm. I see very little substantive differences (solver configuration) but lots of cosmetic differences (comments, subtly different script behavior).

cd examples
git diff --no-index ibpm/cylinder2dRe100_GPU/ decoupledibpm/cylinder2dRe100_GPU/
 {ibpm => decoupledibpm}/cylinder2dRe100_GPU/README.md |  8 ++++----                                                                                                                          
 .../cylinder2dRe100_GPU/config.yaml                   |  3 +++
 .../cylinder2dRe100_GPU/scripts/createBody.py         |  9 +++------
 .../scripts/plotForceCoefficients.py                  | 19 +++++++++----------
 .../cylinder2dRe100_GPU/solversAmgXOptions.info       | 12 ++++++++----
 .../cylinder2dRe100_GPU/solversPetscOptions.info      |  5 +++++
 6 files changed, 32 insertions(+), 24 deletions(-)

This organization makes it more difficult to compare methods when it could have been as easy as running the same test case with different solvers. It's also a lot more to maintain and the small inconsistencies grow over time.

My concern with namespacing is link-time collision. For example, the user or a different library could define a symbol with the same name and yet it might end up calling your version. Either make them static, internal in the sense of GCC __attribute__((visibility("hidden"))), or namespaced.

What do you think about installing a pkg-config file? That is more standard than relying on the libtool file, and makes linking relatively easy for users that don't use libtool (including CMake).

labarba commented 6 years ago

The substantive difference between running a case with ibpm versus decoupledibpm lies in the mathematical formulation, which results in different linear systems (even different number of systems) to solve. This difference reflects in the choice of solver and preconditioner that, in each case, will perform better, given the structure of the matrices. At this point, a user needs to read some papers to understand something about each method.

Since PetIBM is a research tool, users need to do this extra work of studying the methods, together with running the examples we provide. The two method require different solver configuration (explained in the README).

We keep an example folder per executable. This keeps each method self-contained in the repo. (A user may want to work with only one of the methods.) I guess you're saying that it might be nice to have a common set of examples for both methods. It didn't occur to us, and we have no strong preference, but at this point, it feels like unnecessary work to reorganize the repo.

labarba commented 6 years ago

Creating a pkg-config script seems like overkill for this project. Bear in mind, at this point we have no known users outside the group. I suppose if some user appeared who prefers this, we'd ask them to contribute it via PR!

piyueh commented 6 years ago

Regarding the functions not in any namespace (i.e., local functions), we fixed this in commit e38da005. We use anonymous namespace to encapsulate these local functions and force internal linkage only, so API users will not be able to use these local functions.

kyleniemeyer commented 6 years ago

@labarba @jedbrown regarding the pkg-config point, I would argue that one of the purposes of the review here is to make recommendations to make software more user-friendly outside the immediate developers/group, so it seems like a useful suggestion to consider from that point of view.

labarba commented 6 years ago

@kyleniemeyer Reasonable point, in general, but libtools is a perfectly acceptable solution here. No one in our group has gone through the exercise of learning pkg-config, so it would be starting from scratch with that tool.

Users of PetIBM need to also link to PETSc, which provides custom scripts for resolving its dependencies, and the user will need to learn about that anyway. PetIBM has almost the same dependencies as PETSc (only real difference is AmgX).

jedbrown commented 6 years ago

PETSc provides a pkg-config. It isn't a difficult standard and is straightforward to provide from autotools: https://autotools.io/pkgconfig/file-format.html

@piyueh Thanks for that fix. I think these three symbols are still not intended to be exported in a non-petibm namespace:

000000000004c4cd T operator<(MatStencil const&, MatStencil const&)
0000000000068c76 T YAML::convert<PetscBool>::decode(YAML::Node const&, PetscBool&)
0000000000068dfc T YAML::convert<PetscBool>::encode(PetscBool const&)

@labarba On organization of examples, this is certainly not a blocking issue, but I felt it was worth pointing out the maintenance burden (leading to the observed divergence) of separate examples when only a few solver parameters are actually different. I think it would be better for both developers and users to unify everything that can be, but I'm happy to accept the JOSS paper either way.

labarba commented 6 years ago

PETSc does provide pkg-config, but PETSc is a 20+ year old project, with thousands of users around the world. From what we've seen the PETSc trainings and documentation explain how to use the customized makefiles (we use that). The manual mentions only in passing that pkg-config exists (section 15.13 Qt Creator Users).

It may not be a difficult standard, but what is the ROI of adding that now? Is it just "nice to have"?

jedbrown commented 6 years ago

Pkg-config immediately works with CMake, plain makefiles, and any other build system. The libtool files are mainly usable only by autotools. So providing pkg-config significantly reduces the effort and improves reliability of linking for anyone who is not using autotools.

labarba commented 6 years ago

You assert that pkg-config has advantages over libtool, and I hear you. It still sounds as "nice to have" and not a necessity. Given that this submission is 3+ months in the pipeline (high for JOSS), considering also that a majority of accepted JOSS papers report software projects of smaller scale, and submitting that the functionality is already provided (only with a "lesser" tool), I respectfully propose that you don't require this for acceptance.

I propose we open an issue on the repo, but not allocate researcher time right now for this improvement.

As justification, I propose that users of PetIBM do require a higher level of skill than the majority of users of JOSS-published software: it relies on PETSc, it uses GPUs, it solves 3D Navier-Stokes ... We are comfortable with the effort that is required for someone who wants to try PetIBM given the ecosystem they inhabit.

We also plan to continue working on the project. Currently, it has 32 stars on GitHub… we may get some comments on an open issue, if someone else is keen on the improvement.

But for now, I request that you let us publish without it.

piyueh commented 6 years ago

@jedbrown Thank you for pointing out those three functions! These three functions should now be fixed in the current state of joss-review branch (from commit 9e701d).

Actually, we decided to put all YAML converters to private functions, not just the two for PetscBool mentioned. They are not for public API users by current design.

kyleniemeyer commented 6 years ago

It looks like the pkg-config option is something that would be nice for the future, but it seems is not required to build and use the software now. Do you agree @jedbrown? If so, have your other issues been resolved?

jedbrown commented 6 years ago

Yeah, fine by me.On May 8, 2018 17:14, Kyle Niemeyer notifications@github.com wrote:It looks like the pkg-config option is something that would be nice for the future, but it seems is not required to build and use the software now. Do you agree @jedbrown? If so, have your other issues been resolved?

—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.

kyleniemeyer commented 6 years ago

@jedbrown great, thanks!

kyleniemeyer commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

kyleniemeyer commented 6 years ago

@labarba @mesnardo @piyueh a few comments on the paper before we accept:

Can you complete your affiliations? (i.e., add city, state, country)
Generally the first paragraph is a bit long, and can probably be broken into multiple paragraphs which are structured a bit better.
You do not actually define IBM or GPU prior to their first use.
The comment about pressure boundary conditions having "caused many headaches for CFD practitioners!" seems a bit casual/opinionated, and isn't backed up by anything. I would suggest either removing that bit, or explaining/backing it up further (which may not be appropriate in this sort of paper).
The sentence "PetIBM features an operator-based design, so it can be used as a toolbox for researching new solution methods." is not clear to me—can you elaborate a bit more on this?
I think there are a few details mentioned in the README that do not appear in the paper, such as support for multiple immersed bodies and moving bodies with prescribed kinetics; it may help to discuss such support. It also might be nice to mention the strong set of examples included.

mesnardo commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

labarba commented 6 years ago

@kyleniemeyer We're done with our beautiful paper!

kyleniemeyer commented 6 years ago

Wow! Big changes. It is just about there, just a few final edits:

In the appendix, this sentence needs to be fixed (maybe missing "how"?):

Many variants of the immersed-boundary method (IBM) depend on one models the forcing.

and then "system of equation" -> "system of equations" two sentences later, and "as follow" -> "as follows" before equation 4.

With those minor fixes, we'll be good to go.

mesnardo commented 6 years ago

Fixed! Thank you @kyleniemeyer.

mesnardo commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

kyleniemeyer commented 6 years ago

@mesnardo @labarba ok great! At this point please archive the software repo and tell us the associated DOI.

mesnardo commented 6 years ago

@kyleniemeyer PetIBM version bumped to 0.3.1 and archived on Zenodo (10.5281/zenodo.1255132).

kyleniemeyer commented 6 years ago

@whedon set 10.5281/zenodo.1255132 as archive

whedon commented 6 years ago

OK. 10.5281/zenodo.1255132 is the archive.

kyleniemeyer commented 6 years ago

@arfon this package is now accepted and ready to publish!

arfon commented 6 years ago

@jedbrown - many thanks for your review here and to @kyleniemeyer for editing this submission ✨

@labarba - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00558 :zap: :rocket: :boom:

whedon commented 6 years ago

:tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:

If you would like to include a link to your paper from your README use the following code snippet:

[![DOI](http://joss.theoj.org/papers/10.21105/joss.00558/status.svg)](https://doi.org/10.21105/joss.00558)

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:

Volunteering to review for us sometime in the future. You can add your name to the reviewer list here: http://joss.theoj.org/reviewer-signup.html
Making a small donation to support our running costs here: https://www.flipcause.com/secure/cause_pdetails/Mjk3Nzk=

mesnardo commented 6 years ago

@jedbrown Thank you very much for your review! @kyleniemeyer Thank you for editing this submission!

openjournals / joss-reviews