search

ReScience / submissions

ReScience C submissions
28 stars 7 forks source link

[Re] Three-dimensional wake topology and propulsive performance of low-aspect-ratio pitching-rolling plates #55

Closed mesnardo closed 3 years ago

mesnardo commented 3 years ago

Dear Editors,

I would like to request a review for the following replication:

Original article: Three-dimensional wake topology and propulsive performance of low-aspect-ratio pitching-rolling plates

PDF URL: [Re] Three-dimensional wake topology and propulsive performance of low-aspect-ratio pitching-rolling plates Metadata URL: metadata.yaml Code URL: barbagroup/petibm-rollingpitching (doi: 10.5281/zenodo.4733323)

Scientific domain: Computational Fluid Dynamics Programming language: C++ (code application), Python (processing scripts) Suggested editor:

cc @labarba (co-author)

khinsen commented 3 years ago

Thanks for this submission @mesnardo, which I think is our first one in computational fluid dynamics. Our only editor in this field is your co-author @labarba, so we will have to improvise a bit.

I can edit this myself, profiting from many contacts in the CFD community, but I won't be able to start before another two weeks due to existing commitments.

@ReScience/editors Can anyone else handle this, ideally more rapidly than myself?

rougier commented 3 years ago

Sorry for late answer. It's actually quite far from my own expertise. If you can handle it that would be great.

labarba commented 3 years ago

Maybe the author of this published ReScience paper could contribute a peer review here, and thus give back to the efforts of the journal? https://github.com/ReScience/submissions/issues/19

chengyu1018 commented 3 years ago

This is Chengyu Li (chengyu.li@villanova.edu), the author of this ReScience paper. I am happy to serve as a reviewer for this paper.

labarba commented 3 years ago

To clarify, Dr. Chengyu Li is the first author of the original article from 2016 (https://doi.org/10.1063/1.4954505) this is a replication of. He was not involved in the replication and we have not interacted before, but I reached out by email this week to ask if he might be interested in reviewing the replication.

khinsen commented 3 years ago

I am back, sorry for the delay. Since nobody else volunteered to edit this in the meantime, I officially proclaim myself handling editor for this submission.

khinsen commented 3 years ago

@chengyu1018 Welcome to ReScience, and thanks for volunteering as a reviewer. I can't think of anyone better placed to do a review! If you have any questions about how ReScience functions, don't hesitate to ask. To start, here are our reviewing guidelines.

Normally, we expect reviewers to verify the reproducibility of the submitted code. This could be difficult in this case because the computations require computing resource that are not on every scientist's desk. See what you can do, and if there's any problem due to the resource requirements, we will adjust. This is our first HPC submission as far as I can see.

khinsen commented 3 years ago

@pantale We received our second submission from engineering, after the reproduction report you submitted last year. Would you be interested in reviewing this work?

pantale commented 3 years ago

@khinsen Hello Konrad, I would be very happy to take part in this review. I am not an expert in numerical simulation in fluid mechanics, my field of expertise is in solid mechanics, nevertheless, I think that with the help of the first author of the original article I can bring my point of view on this replication even if I have neither the material means nor the expertise to carry out the replication of this computation by myself. Tell me what you decide. Olivier

khinsen commented 3 years ago

@pantale Thanks for willingness to review! I'd say go ahead, we will see how this works out. We will have to figure out one day how to deal with HPC submissions, so "now" looks like a good moment :-)

pantale commented 3 years ago

@khinsen Hello, Difficult for me to evaluate properly the proposed paper, but are some remarks:

The authors take care in the introduction to clearly distinguish the notion of replication from the notion of reproduction, which is a plus for the positioning of the problematic of the article. The source code and the data of the original article are not available, so it is indeed a reproduction. The authors took care to make available on github all the data and sources used for the production of the article.

Points of detail: The authors use bibliographic references and footnotes in the text, with exactly the same formalism of recall for both types of reference, making it difficult to read. For example, on page 2, Li and Dong5 refer to the reference, while github1 refers to the footnote. These two types of notation should be distinguished.

Methodology: On page 4, the authors state that in the original paper by Li and Dong, the method for calculating hydrodynamic power is not specified. They bring their own solution and specify that "the surface pressure is defined as the fluid pressure interpolated at a distance of 3%", what is then the influence of this value on the reproducibility of the result, and why such a choice?

The hardware configuration used is not clear enough for me and differs between the abstract and section 3. The number of CPUs and Nvidia GPUs involved in the simulations is not clear from the article, as well as the RAM and GPU memory size.

Overall assessment: Even though I am not a fluid mechanics specialist myself (my field of application is solid mechanics), I appreciated the detailed content of the article and the efforts provided by the authors to give as much detail as possible of the implementation and the different tracks allowing again to reproduce these simulations. The notable efforts concerning the provision of Docker images, simulation data, python scripts allowing the generation of figures in the article is a notable plus.

So without being a specialist in this type of numerical simulation, I think we can accept the proposed article with minor revisions for publication in ReScience C.

Olivier

khinsen commented 3 years ago

Thanks @pantale !

@mesnardo Feel free to comment/reply to this review now, or wait for the second one and deal with both at the same time.

khinsen commented 3 years ago

@mesnardo Something I can't find in your paper is the minimal hardware requirements to (1) run your Docker/singularity image or (2) run the whole code from source code, i.e. including a rebuild of the image. You do say "We used computational nodes with Dual 20-Core 3.70GHz Intel Xeon Gold 6148 processors and NVIDIA V100 GPU devices." but that doesn't mean that an exactly identical machine is required for replication. Would any recent Intel-based machine be OK? Are the GPUs required? Is the specifc type of GPU required?

rougier commented 3 years ago

@mesnardo Any progress?

mesnardo commented 3 years ago

@khinsen Thank you for the feedback. We will add more details in the manuscript (or in the README of the application repository).

Although we have not tried it, I believe the Singularity image should run on a more recent Intel-based machine. If it does not, we at least made the Dockerfiles and Singularity recipes available, so that one could adapt/re-build the images:

The Docker image contains a version of PetIBM that enables the use of GPUs. However, if no GPU devices are available on the host, one should still be able to use the same image and solve all iterative solvers on CPUs (with PETSc). (We only used GPUs to solve the pressure Poisson system in our simulations; the two other systems were solved on CPUs.)

The Docker image contains a version of AmgX that was compiled to target GPUs with a compute capability of 3.5 (Kepler - Tesla K20, K40), 3.7 (Kepler - Tesla K80), 6.0 (Pascal - Tesla P100), and 7.0 (Volta - Tesla V100).

@rougier We would like to wait for the second review before making changes to the manuscript and replying to the reviewers.

chengyu1018 commented 3 years ago

@khinsen

I have read the manuscript entitled, “[Re] Three-dimensional wake topology and propulsive performance of low-aspect-ratio pitching-rolling plates.” The authors successfully replicated the simulation results of the original study (Li & Dong, Physics of Fluids, 2016) with a different CFD solver. Although the force coefficient peaks showed a slightly lower magnitude than the original paper, the most important characteristic features were well captured, such as “double-C” -shaped vortex structures and force double peaks. It is worth noting that the “double-C”-shaped vortex structures were first reported by Li & Dong in the literature. Best of the reviewer’s knowledge, there have been no experimental studies that successfully observed this structure due to the complex wake structures generated by the bio-inspired pitching-rolling motion. So, the reviewer believes that the current Re-Science CFD study provides important evidence on the existence of such a unique wake structure since the numerical treatment between the original article and the Re-Science paper are very different. In addition, the overall observation on the effects of Reynolds number, Strouhal number, Aspect-ratio are consistent with the original article. I am not surprised by the difference in the force coefficient between the two studies. Depending on the numerical schemes used in different CFD solvers, some commercial/open-source software packages (e.g., Ansys Fluent) may introduce artificial viscosity for better convergence reasons.

The overall contents are well written and accessible to the broad readership of ReScience C. I only have one minor suggestion. It would be nice if the authors could consider adding a plot to demonstrate the mesh used in the current Re-Science study (like Fig.3 in Li & Dong’s paper).

khinsen commented 3 years ago

Thanks @chengyu1018 for your detailed review, and thanks @mesnardo for answering my questions. Unfortunately my agenda is very dense this week, so please be patient for a few more days for me to re-read everything carefully!

khinsen commented 3 years ago

@mesnardo Since the reviewers didn't comment on the reproducibility, I am looking at this myself. At least theoretically (inspecting the files), and running as much as I can with the machines I have at my disposal.

I started with the easiest part: re-running your "repro packs". Using Docker for macOS, I can follow your instructions and I get freshly generated figures. However, only the line plots look the same. The 3D visualizations are not rendered at all, the figures contain nothing but the arrows and lines, often just the coordinate axes in the corner. One possible explanation is that the Docker image is made for NVIDIA graphics cards, which I don't have in my computer. If that's indeed the cause, I do find it worrying from a reproducibility perspective. If the code requires hardware (or systems software) that is not present, it should fail with an error message, instead of silently producing wrong output.

Next, I will try rebuilding the Docker and Singularity images. The instructions for that seem clear. The remaining step would be re-running the simulations themselves, which I might not be able to do. The instructions look clear enough, but I am not sure to be able to find suitable hardware.

khinsen commented 3 years ago

Update: I can't build the petibm container image following the provided instructions: https://github.com/barbagroup/petibm-recipes/issues/3

mesnardo commented 3 years ago

@khinsen Thank you for attempting to build the image and opening an issue on the repo. I'm away from keyboard this week, but will get back to work next Monday and investigate the issues about the 3D visualization and the Docker image. We will also address the feedback from the reviewers.

khinsen commented 3 years ago

@mesnardo Here's another question for you (for when you are back, it's not urgent!), more about improving ReScience C than about your submission.

Over here, @benureau proposes a way to improve the reviewing process for HPC-style submissions. I'd like to have your view as an author on this idea. Would such a process be a reasonable burden on authors of future submissions? Note that I am not asking you to adopt this for the current submission, it's only about future submissions.

labarba commented 3 years ago

Note that the code that was used in our submission here was separately submitted, reviewed and published in JOSS. This means that reviewers installed the code, ran tests, and gave input to improve it. We have also made deposits with the primary data, as suggested by @benureau. And provide the secondary data (often smaller files, due to being summarized in some form) so the reproduction of figures is facilitated.

In a high-performance scenario, we need to shift our expectations of reproducibility being exercised and view it as prospective reproducibility, demonstrated by the documented process (how the research was conducted), and artifacts.

Related blog post from 2016: "Why should I believe your supercomputing research?"

khinsen commented 3 years ago

The JOSS paper is a good point, thanks @labarba! It would be helpful to point this out in the paper. The mere reference to a JOSS paper probably doesn't mean much for most readers (and reviewers). But it does indeed imply that someone has done an open review of the software, including installation and running the tests. We could even suggest to future authors to provide a link to a JOSS review if one exists for software they used. Let's start citing reviews!

And I fully agree of course that we need to shift our expectations - that's exactly what the discussion at https://github.com/ReScience/ReScience/issues/104 is about.

labarba commented 3 years ago

Dear editor and reviewers: we have made changes responding to reviewer comments in this merged PR https://github.com/barbagroup/rescience-rollingpitching/pull/3

mesnardo commented 3 years ago

@khinsen, @pantale, @chengyu1018 Thank you for the valuable feedback you provided! We addressed your comments/questions with modifications to the manuscript and to the code application repository:

(The revised manuscript also adds a thank you to the reviewers and editor in the "Acknowledgments" section; in barbagroup/rescience-rollingpitching@a5e4ef4.)

mesnardo commented 3 years ago

Replying to @chengyu1018

I only have one minor suggestion. It would be nice if the authors could consider adding a plot to demonstrate the mesh used in the current Re-Science study (like Fig.3 in Li & Dong’s paper).

That's a great suggestion; it will give a visual sense to the readers on the type of grids used in this study. We added a figure containing a sub-view of the mesh grid (for the base case) to the manuscript (Figure 1). Figure added to manuscript in barbagroup/rescience-rollingpitching@b0501c6. Script to generate the figure added to the application repo in barbagroup/petibm-rollingpitching@41eaabc.

mesnardo commented 3 years ago

Replying to @pantale

The authors use bibliographic references and footnotes in the text, with exactly the same formalism of recall for both types of reference, making it difficult to read. For example, on page 2, Li and Dong5 refer to the reference, while github1 refers to the footnote. These two types of notation should be distinguished.

100% agree. To distinguish between references and footnotes, we now use symbols as footnote markers. The switch to symbols was done in barbagroup/rescience-rollingpitching@aad7e5d.

On page 4, the authors state that in the original paper by Li and Dong, the method for calculating hydrodynamic power is not specified. They bring their own solution and specify that "the surface pressure is defined as the fluid pressure interpolated at a distance of 3%", what is then the influence of this value on the reproducibility of the result, and why such a choice?

We added to the manuscript a more detailed paragraph on the calculation of the hydrodynamic power. In this paragraph, we explain why we choose to interpolate at a distance of 3% of the chord length and provide numerical comparison to when interpolating closer or further away from the boundary. The revised paragraph was added to the manuscript in barbagroup/rescience-rollingpitching@db55870.

The hardware configuration used is not clear enough for me and differs between the abstract and section 3. The number of CPUs and Nvidia GPUs involved in the simulations is not clear from the article, as well as the RAM and GPU memory size.

I may have not understood your comment about the difference in reporting the hardware configuration (between the abstract and section 3). However, we agree we should provide more information about the hardware configuration used for the simulations. In barbagroup/rescience-rollingpitching@d52b864, we added (to the manuscript) a pointer for the readers on where to find the hardware configuration for each simulation (in the README of the application repo). The README of the application repo contains a table with details about the hardware used for each simulation. The table was added in barbagroup/petibm-rollingpitching@60eb292.

mesnardo commented 3 years ago

Replying to @khinsen

Something I can't find in your paper is the minimal hardware requirements to (1) run your Docker/singularity image or (2) run the whole code from source code, i.e. including a rebuild of the image. You do say "We used computational nodes with Dual 20-Core 3.70GHz Intel Xeon Gold 6148 processors and NVIDIA V100 GPU devices." but that doesn't mean that an exactly identical machine is required for replication. Would any recent Intel-based machine be OK? Are the GPUs required? Is the specific type of GPU required?

We added a section to the README of the code repository with the requirements (software/hardware) to run the Singularity container (barbagroup/petibm-rollingpitching@a06bec9). And considering that most of us do not have free access to machines with enough memory, we also added some instructions to launch and configure an AWS EC2 instance to run the simulations (barbagroup/petibm-rollingpitching@061f755).

The 3D visualizations are not rendered at all, the figures contain nothing but the arrows and lines, often just the coordinate axes in the corner. One possible explanation is that the Docker image is made for NVIDIA graphics cards, which I don't have in my computer. If that's indeed the cause, I do find it worrying from a reproducibility perspective. If the code requires hardware (or systems software) that is not present, it should fail with an error message, instead of silently producing wrong output.

Starting from the repro-packs downloaded from Zenodo, I tried to re-generate the figures on my laptop (8GB of memory). The process also failed to render the 3D visualizations on my laptop. Digging into this issue, it appears that 8GB of memory is not enough to compute the secondary data (such as the 3D vorticity field). (Looking into the log, I saw that the process for the application petibm-vorticity got killed.) I then used an AWS EC2 instance (t3.2xlarge, 32 GB of memory) to confirm I could correctly generates the figures. (You do not need access to NVIDIA GPUs to generate the secondary data and figures.) It appears that a machine with more than 16GB of memory is required to generate the figures. In barbagroup/petibm-rollingpitching@2727b85, we added a warning (to the README) about the memory requirement for generating secondary data and generating the figures. In this commit, we also added some instructions to launch an AWS EC2 instance and run the Docker container to generate the data and figures. (Anyone with a AWS account should now be able to download the repro-packs from Zenodo, to download the Docker images from Zenodo, and to execute the Docker container.)

The JOSS paper is a good point, thanks @labarba! It would be helpful to point this out in the paper. The mere reference to a JOSS paper probably doesn't mean much for most readers (and reviewers).

100% agree. In barbagroup/rescience-rollingpitching@e7f754a, we highlighted the JOSS paper and what it means for a software to be accepted and published in JOSS.

Update: I can't build the petibm container image following the provided instructions: barbagroup/petibm-recipes#3

Thanks again for opening an issue on barbagroup/petibm-recipes#3. There, you raised very good points about lacking reproducible builds for the Docker images (apt update, relying on base images that could be altered, etc.). Thus, we deposited to Zenodo the Docker and Singularity images used for this study DOI. In barbagroup/petibm-rollingpitching@c03805b, we added the DOI to the README of the application repo (as well as the version used for Docker and Singularity). We are also now citing the Zenodo deposit in the manuscript (barbagroup/rescience-rollingpitching@533466a). In addition to that, we added sentences to the last section to explain why re-building a Docker image may fail or may not be reproducible (barbagroup/rescience-rollingpitching@533466a).

khinsen commented 3 years ago

Thanks @mesnardo for those very useful and relevant additions to your code and paper, and also for summarizing them here with precise references so that it's easy for us to find them. With the right tools and the right attitude, reviewing papers can actually be a nice experience!

@chengyu1018 and @pantale: please have a look at these changes and say if you are happy with them.

khinsen commented 3 years ago

@mesnardo As for the changes prompted by my own comments, they look very good. Providing AWS instructions is an excellent idea for helping others build on your work, as is archiving the container images. I am not much of a fan of that approach in general, but as a workaround for difficult situations, that's fine with me.

The memory requirements do indeed explain my failure to reproduce the repro-pack. My laptop has 8GB, meaning that the virtual machine that Docker images are run with (under macOS) gets even less.

chengyu1018 commented 3 years ago

@khinsen The authors have adequately addressed my concerns and I believe this is acceptable for publication.

pantale commented 3 years ago

This is also Ok for me.

labarba commented 3 years ago

Sincere thank you to both reviewers and the editor for their contribution to this paper's publication in ReScience.

khinsen commented 3 years ago

Thanks @chengyu1018 and @pantale for reviewing this paper, which is now officially accepted! And thanks to @mesnardo and @labarba for providing ReScience with this interesting challenge to its reviewing procedure.

I'll start with the technicalities of publication, and it is possible that I will request help from @mesnardo. Stay tuned.

khinsen commented 3 years ago

Status update: Looks like I won't get this out today - Software Heritage seems to be very slow, so I don't have the SWHID for the code repository yet.

khinsen commented 3 years ago

@mesnardo I tried to recompile your LaTeX file after updating the metadata. It works but I get suspicious warnings about multiply defined labels:

LaTeX Warning: Label `fig:independence_force_coefficients_dx' multiply defined.
LaTeX Warning: Label `fig:independence_force_coefficients_dx@cref' multiply def
ined.

Could you please fix this, or confirm that I can safely ignore the warnings?

khinsen commented 3 years ago

@mesnardo Alternatively, accept https://github.com/barbagroup/rescience-rollingpitching/pull/4 and generate the final PDF yourself!

mesnardo commented 3 years ago

@khinsen Thank you for the PR. I merged it and generated the PDF (after removing the duplicated figure, which was causing the warning). The PDF can be found here.

khinsen commented 3 years ago

The article has been published: http://doi.org/10.5281/zenodo.5234931. It will be listed on the ReScience Web site after the next update.

labarba commented 3 years ago

I'm a little confused by the footer indicating volume and issue. Is it Volume 7 issue 1 ? What do you use in ReScience for "page number"? Online-only journal generally use some article id for that.

khinsen commented 3 years ago

7.1 (#7) means volume 7, issue 1, article 7. The last 7 is thus the article id.