search

openjournals / joss-reviews

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

[REVIEW]: turtleFSI: A Robust and Monolithic FEniCS-based Fluid-Structure Interaction Solver #2089

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @albansouche (Alban Souche) Repository: https://github.com/KVSlab/turtleFSI.git Version: v1.4 Editor: @Kevin-Mattheus-Moerman Reviewers: @chennachaos, @JaroslavHron Archive: 10.5281/zenodo.3895443

Status

status

Status badge code:

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

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

@nathanquinlan & @chennachaos & @JaroslavHron, 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 @Kevin-Mattheus-Moerman know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @chennachaos

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @JaroslavHron

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @nathanquinlan, @chennachaos, @JaroslavHron it looks like you're currently assigned to review 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:

  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 4 years ago 
Reference check summary:

OK DOIs

- 10.1146/annurev.fluid.30.1.539 is OK
- 10.1023/A:1020843529530 is OK
- 10.1016/S0045-7825(00)00381-9 is OK
- 10.1016/j.compstruc.2011.02.019 is OK
- 10.1007/978-3-642-23099-8 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

Kevin-Mattheus-Moerman commented 4 years ago

@nathanquinlan, @chennachaos, @JaroslavHron, this is where the review takes place.

The instructions and checkboxes at the top will guide you through the review process (see our review criteria for more information). As a reviewer you can leave comments on this work in this thread. For larger issues we encourage you to open issues on the project repository and to link to them here in a comment.

Let me know if you have questions. Thanks again for your help! :rocket: :robot:

Kevin-Mattheus-Moerman commented 4 years ago

@nathanquinlan, @chennachaos, @JaroslavHron, thanks for your help with this review! Can you provide an update on review progress? Thanks!

chennachaos commented 4 years ago

Hi @Kevin-Mattheus-Moerman. Apologies for the delay. I will try to complete the review by the end of this month.

arfon commented 4 years ago

Dear authors and reviewers

We wanted to notify you that in light of the current COVID-19 pandemic, JOSS has decided to suspend submission of new manuscripts and to handle existing manuscripts (such as this one) on a "best efforts basis". We understand that you may need to attend to more pressing issues than completing a review or updating a repository in response to a review. If this is the case, a quick note indicating that you need to put a "pause" on your involvement with a review would be appreciated but is not required.

Thanks in advance for your understanding.

Arfon Smith, Editor in Chief, on behalf of the JOSS editorial team.

chennachaos commented 4 years ago

Hi @albansouche I must say that it is very refreshing to see an open-source software library for FSI problems.

I have started the review. I have been going through the code and the associated documentation. I am having difficulties in understanding some parts. I would like to understand them at length before providing my detailed remarks.

1.) How, and on what level, does turtleFSI interact with FEniCS?

2.) What is the example problem used in the tests?

3.) Can the solver cope with mismatching meshes at the fluid-solid interface?

4.) Do you have any data on the convergence of Newton-Raphson iterations and how this convergence is affected under different settings of parameters passed to the newtonsolver?

5.) a) How is the Jacobian matrix for the monolithic scheme computed? b) How is the matrix system resulting from the monolithic scheme solved?

These (5a and 5b) are the two most time-consuming parts in an FSI solver. To gain insights into the computational aspects, I have looked at the graphs you have nicely prepared in the solver performance section. From the graphs, I notice that one assembly of the Jacobian matrix requires about two orders of magnitude more CPU time than that of one matrix solver. My observation seems to be in line with your statement "Notice that most of the compute time is spent on the Jacobian operations" in the caption.

This behaviour of the solver, however, is counter-intuitive to me because the matrix solver should be the most expensive part in any implicit scheme. Please clarify why the formation of the Jacobian matrix is so much more expensive.

albansouche commented 4 years ago

Hi @chennachaos Thank you for your first feedbacks. Here are my replies.

1.) How, and on what level, does turtleFSI interact with FeniCS? turtleFSI interacts with FEniCS through the python interface. It provides a compact code relatively easy to modify for further development for anyone with basic knowledge of the FEniCS library.

2.) What is the example problem used in the tests? We check all the examples (Turek flag with only solid, only fluid, and FSI) and all the mesh lifting operators in the tests.

3.) Can the solver cope with mismatching meshes at the fluid-solid interface? Unfortunately not. turtleFSI solver only copes with conforming solid and fluid meshes at the interface.

4.) Do you have any data on the convergence of Newton-Raphson iterations and how this convergence is affected under different settings of parameters passed to the newtonsolver? We didn’t document this aspect in details since the convergence rate can vary greatly depending on the problem being solved, the mesh resolution, or the time step…
In brief, the convergence is almost systematical quadratic when the Jacobian matrix is re-evaluated at each iteration. However, for efficiency reason, most of the time we prefer to reuse the Jacobian matrix over the Newton iterations even if this results in a sub-linear convergence rate. Recomputing the Jacobian matrix is not excessively time consuming by itself, but will systematically require a new factorization step within the direct solver (here, MUMPS) which is computationally very expensive.

5.) a) How is the Jacobian matrix for the monolithic scheme computed? The Jacobian matrix is numerically evaluated within FEniCS.

b) How is the matrix system resulting from the monolithic scheme solved? It is solved with MUMPS solver available though the FEniCS PETSc backend. It appears to be the most robust solver available from FEniCS for FSI problems.

These (5a and 5b) are the two most time-consuming parts in an FSI solver. To gain insights into the computational aspects, I have looked at the graphs you have nicely prepared in the solver performance section. From the graphs, I notice that one assembly of the Jacobian matrix requires about two orders of magnitude more CPU time than that of one matrix solver. My observation seems to be in line with your statement "Notice that most of the compute time is spent on the Jacobian operations" in the caption. This behaviour of the solver, however, is counter-intuitive to me because the matrix solver should be the most expensive part in any implicit scheme. Please clarify why the formation of the Jacobian matrix is so much more expensive.

This is a very good observation and we also investigated this point further since the submission. Indeed the graphs presented in documentation (Solver performance) contain a mistake and will be rectified. The reported time for the Jacobian matrix evaluation and assembly was taken together with the preprocessing step of the MUMPS solver, which is obviously a mistake. The preprocessing step in MUMPS corresponds to the factorization of the stiffness matrix of the linear system of equations. As to be expected, the factorization is the most memory and time consuming step for the direct solver. As mentioned in reply 4.), the computation and assembly of the Jacobian matrix is not the limiting factor regarding computational time, but will systematically require a new factorization within the direct solver which is computationally very expensive (roughly 2 orders of magnitude in comparison to the solving of the equations). There might be some options to interact with MUMPS directly through PETSc to control the re-factorization of the matrix for a given sparsity pattern. We have not investigated this part further at the moment.

JaroslavHron commented 4 years ago

Hi @albansouche , your turtleFSI is really nice tool to easily setup and run FSI problems.

albansouche commented 4 years ago

Hi @JaroslavHron, Thank you for the nice feedback and comments. Here are my replies:

Comment 1: Yes, theta=1 corresponds to a fully implicit scheme, theta=0 to a fully explicit scheme, and theta=0.5 to a Crank-Nicolson scheme. One may also use the so-called shifted Crank-Nicolson scheme that corresponds to 0.5 + dt (the time step), “which allows for global stability of the solution” (Wick, 2011) and preserves the convergence properties of the Crank-Nicolson scheme. In the examples we use a shifted Crank-Nicolson scheme with value of 0.51 when dt is set to 0.01 s. You can find the reference here: Wick, T., 2011. Adaptive finite element simulation of fluid-structure interaction with application to heart-valve dynamics (Doctoral dissertation). https://doi.org/10.11588/heidok.00012992

albansouche commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

albansouche commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

albansouche commented 4 years ago

Hi @chennachaos, Following your comment on the computational cost of the different parts of the solver, I modified some of the text and the figure present in the documentation (https://turtlefsi2.readthedocs.io/en/latest/verif_perf.html).

chennachaos commented 4 years ago

Thanks for your response, @albansouche.

I have the following comments regarding the software paper.

chennachaos commented 4 years ago

I still have some issues in understanding the computational times reported in the "Solver performance" page. In a tetrahedral mesh with 63,000 first-order elements, there would be about 15000 nodes. Based on this, I estimate the total DOFs for the monolithic scheme to be about 60,000. My experience indicates that the matrix factorisation step should not take 1000 seconds (~16 minutes) (assuming that the optimisation flags are turned ON) for a matrix of this size. I tried to understand this further from the graph, but it is not clear to me what "jac.(1) + fact.(1) + it.(5)" represents. Does it mean that you performed 5 iterations by computing and factorising the Jacobian matrix once? If this the case, then (a) how does the error norm of the residual converge and (b) what is the tolerance for convergence criterion?

I would like to see the computational time split into three parts: (i) for computing the Jacobian, (ii) for the factorisation and (iii) for the solver, if you reuse the Jacobian matrix, for the first 100 time steps. Also, please do provide the convergence of the residual norm in Newton-Raphson iterations for a few random time steps in the first 100 time steps.

aslakbergersen commented 4 years ago

@chennachaos, thank you for your comments. We apologize for the delayed response.

Thanks for your response, @albansouche.

I have the following comments regarding the software paper.

  • Summary: is nicely written.
  • A statement of need: It is not clear what the motivation is and who the target audience are. Although the monolithic FSI solvers are robust, they are notorious to be computationally expensive beyond 100,000 DOFs, even for benchmark problems. Please note that benchmark problems are not always simple; one requires to run a considerable number of simulations (i) to assess the convergence of overall scheme, (ii) how different ingredients affect the accuracy, convergence and robustness, and (iii) to evaluate the parallel performance of the software tool.
  • State of the field: This aspect is missing from the paper. There is no discussion and comparison with other FSI solvers (codes) available today. Oomph-lib and Elmer FEM are two such full-fledged open-source solvers. PreCICE is an open-source coupling library for FSI problems. Why should one prefer this FSI solver over others?
  • Quality of writing: The paper is well written.
  • References: The list of references needs to be improved by adding some classical references in the field of FSI.

To address the above points, we have added the text following below to the paper, with changes highlighted in bold. We have further specified the motivation, target audience, and why some could prefer turtleFSI over other solvers. We have also added some reference to other open-source solvers with FSI capabilities:

(…), but where physical insight might suffice. The aim was therefore to develop a fully monolithic and robust entry-level research code with ease-of-use targeted towards students, educators, and researchers.

FEniCS [@Logg:2012] has emerged as one (…) FEniCS was a natural choice of computing environment. Compared to other open-source FSI solvers [@Malinen:2013; @Heil:2006; @Jasak:2007], turtleFSI is written in only a couple of hundred lines of high-level Python code, in contrast to tens of thousands of lines of low-level C++ code. This provides full transparency and a unique opportunity for researchers and educators to modify and experiment with the code, while still providing out of the box entry-level high-performance computing capabilities. Furthermore, because of the close resemblance between mathematics and code in FEniCS, users can make additions or modifications with ease.

The turtleFSI solver rely on a fully monolithic (…) which can easily be adapted to other 2D or 3D FSI problems.

In conclusion, turtleFSI is not a superior FSI solver in terms of speed, but it is a robust entry-level FSI solver and performs exactly as designed and intended; ‘slow and steady wins the race’

We have extended the section Solver performance from the documentation with a new section Newton solver convergence and performance. The documentation now includes a comparison of the Newton solver convergence between performing a full update and reusing the Jacobian matrix. This should answer the questions and concerns quoted below on the convergence behavior of the Newton solver used in different conditions. Of course, the reported behavior is not unique and depends on the geometry and boundary conditions but reflects well the general behavior we have observed using turtleFSI in our research work.

I still have some issues in understanding the computational times reported in the "Solver performance" page. In a tetrahedral mesh with 63,000 first-order elements, there would be about 15000 nodes. Based on this, I estimate the total DOFs for the monolithic scheme to be about 60,000.

Our 3D FSI pipe flow problem has 460,000 dofs when discretized with 63000 tetrahedral elements. The pressure is approximated with a continuous linear scalar field, but the velocity and the deformation are both solved with a continuous quadratic vector field for stability reason.

My experience indicates that the matrix factorisation step should not take 1000 seconds (~16 minutes) (assuming that the optimisation flags are turned ON) for a matrix of this size. I tried to understand this further from the graph, but it is not clear to me what "jac.(1) + fact.(1) + it.(5)" represents. Does it mean that you performed 5 iterations by computing and factorising the Jacobian matrix once?

Yes, we compute and factorize the Jacobian matrix once at the first iteration and reused it for the next iterations. The factorization of the Jacobian matrix (with size 460000*460000) takes indeed ca. 1000 seconds running on 1 core and 1 thread.

If this the case, then (a) how does the error norm of the residual converge and (b) what is the tolerance for convergence criterion? I would like to see the computational time split into three parts: (i) for computing the Jacobian, (ii) for the factorisation and (iii) for the solver, if you reuse the Jacobian matrix, for the first 100 time steps. Also, please do provide the convergence of the residual norm in Newton-Raphson iterations for a few random time steps in the first 100 time steps.

We have performed additional runs and documented the solver convergence rate in the new version of the section “Newton solver convergence and performance” of the software documentation.

aslakbergersen commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

Kevin-Mattheus-Moerman commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1146/annurev.fluid.30.1.539 is OK
- 10.1023/A:1020843529530 is OK
- 10.1016/S0045-7825(00)00381-9 is OK
- 10.1016/j.compstruc.2011.02.019 is OK
- 10.1007/978-3-642-23099-8 is OK
- 10.1007/3-540-34596-5_15 is OK

MISSING DOIs

- https://doi.org/10.1007/3-540-34596-5_2 may be missing for title: oomph-lib–an object-oriented multi-physics finite-element library

INVALID DOIs

- None
Kevin-Mattheus-Moerman commented 4 years ago

@albansouche @aslakbergersen can you check for that potentially missing DOI? thanks.

aslakbergersen commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1146/annurev.fluid.30.1.539 is OK
- 10.1023/A:1020843529530 is OK
- 10.1016/S0045-7825(00)00381-9 is OK
- 10.1016/j.compstruc.2011.02.019 is OK
- 10.1007/978-3-642-23099-8 is OK
- 10.1007/3-540-34596-5_15 is OK

MISSING DOIs

- None

INVALID DOIs

- https://doi.org/10.1007/3-540-34596-5_2 is INVALID because of 'https://doi.org/' prefix
aslakbergersen commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1146/annurev.fluid.30.1.539 is OK
- 10.1023/A:1020843529530 is OK
- 10.1016/S0045-7825(00)00381-9 is OK
- 10.1016/j.compstruc.2011.02.019 is OK
- 10.1007/978-3-642-23099-8 is OK
- 10.1007/3-540-34596-5_15 is OK
- 10.1007/3-540-34596-5_2 is OK

MISSING DOIs

- None

INVALID DOIs

- None
Kevin-Mattheus-Moerman commented 4 years ago

@nathanquinlan, @chennachaos, @JaroslavHron thanks for your review efforts! We greatly appreciate your time and help!

@chennachaos It looks like the authors made a number of changes which look ready for you to review.

@JaroslavHron your last unticked box relates to references, I believe these have now been fixed. Can you check and if possible tick the last box? Thanks!

@nathanquinlan are you able to start with your part of the review process? Thanks!

aslakbergersen commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

chennachaos commented 4 years ago

Hi @aslakbergersen Thank you for your detailed response and for your updates to the paper and the documentation.

chennachaos commented 4 years ago

I have checked the installation process and found that the installation is quite involved especially if one does not have anaconda installed. (Conda requires about 4-5 GB disk space).

However, I have used the development version and installed it by following the instructions in the documentation page. Everything worked without any issues. I was also able to run the default test case; I had to kill the job though, as the default settings (3000 time steps) requires a lot of time to run. My suggestion is to modify the default settings for the test cases so that the overall simulation time for the test cases is limited to a few minutes, perhaps with an accompanying note to the user.

It is fantastic that you included the well-known benchmark FSI example from the Turek and Hron paper. However, test cases with the time-periodic response of the structure require a significant amount of time to get some meaningful results beyond the onset of vortex-shedding. To make it easy for the novice users to understand the code and run test cases, I suggest including a numerical example with a steady-state response, for example, a beam in crossflow, as studied in https://www.sciencedirect.com/science/article/pii/S0045782518301026

aslakbergersen commented 4 years ago

Hi @chennachaos, I apologize for the delay. See below for answers to your comments.

I have checked the installation process and found that the installation is quite involved especially if one does not have anaconda installed. (Conda requires about 4-5 GB disk space).

Thank you for your diligence. We have added a note that recommends users to install miniconda over the full anaconda, and provide the following link for users to learn more about the difference.

However, I have used the development version and installed it by following the instructions in the documentation page. Everything worked without any issues. I was also able to run the default test case; I had to kill the job though, as the default settings (3000 time steps) requires a lot of time to run. My suggestion is to modify the default settings for the test cases so that the overall simulation time for the test cases is limited to a few minutes, perhaps with an accompanying note to the user.

It is fantastic that you included the well-known benchmark FSI example from the Turek and Hron paper. However, test cases with the time-periodic response of the structure require a significant amount of time to get some meaningful results beyond the onset of vortex-shedding. To make it easy for the novice users to understand the code and run test cases, I suggest including a numerical example with a steady-state response, for example, a beam in crossflow, as studied in https://www.sciencedirect.com/science/article/pii/S0045782518301026

This is an excellent point we as developers did not think about. The turtle_demo.py setup was initially designed to simulate a turtle swimming over many cycles to check the stability of the setup and create the animation for the repository. We have now reduced the total time of the simulation of the turtle demo problem to execute only the first 30 time steps, which should run in a couple of minutes on a desktop computer. We have also added a note in the documentation to explain this further.

chennachaos commented 4 years ago

Hi @aslakbergersen. Thank you for response and for the changes you made to the code and the documentation.

chennachaos commented 4 years ago

HI @Kevin-Mattheus-Moerman. I now have completed my review.

The authors have addressed all the issues and improved the quality of code, tests and documentation to a satisfactory level. I recommend the turtleFSI library for publication in the JOSS.

Thank you very much for the opportunity!

Kevin-Mattheus-Moerman commented 4 years ago

Thanks @chennachaos for your help here!!!!

Kevin-Mattheus-Moerman commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

nathanquinlan commented 4 years ago

Dear all, Sorry I haven't been able to contribute to the review process to date. I don't want to delay the process further so I have no objection if the editors would like to proceed without my review. Regards Nathan

Kevin-Mattheus-Moerman commented 4 years ago

Thanks for letting us know @nathanquinlan

Kevin-Mattheus-Moerman commented 4 years ago

@whedon remove @nathanquinlan as reviewer

whedon commented 4 years ago

OK, @nathanquinlan is no longer a reviewer

Kevin-Mattheus-Moerman commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1146/annurev.fluid.30.1.539 is OK
- 10.1023/A:1020843529530 is OK
- 10.1016/S0045-7825(00)00381-9 is OK
- 10.1016/j.compstruc.2011.02.019 is OK
- 10.1007/978-3-642-23099-8 is OK
- 10.1007/3-540-34596-5_15 is OK
- 10.1007/3-540-34596-5_2 is OK

MISSING DOIs

- None

INVALID DOIs

- None
Kevin-Mattheus-Moerman commented 4 years ago

Thanks @chennachaos @JaroslavHron for your review efforts! We will now process this work for acceptance in JOSS

Kevin-Mattheus-Moerman commented 4 years ago

@albansouche I will now process your paper for acceptance.

Here are some comments on your paper:

Once you've made the above changes you can run @whedon generate pdf here to update the paper.

Finally please also do the following:

aslakbergersen commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

aslakbergersen commented 4 years ago

@whedon generate pdf