search

openjournals / joss-reviews

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

[REVIEW]: fib-tf: A TensorFlow-based Cardiac Electrophysiology Simulator #719

Closed whedon closed 6 years ago

whedon commented 6 years ago

Submitting author: @siravan (Shahriar Iravanian) Repository: https://github.com/siravan/fib_tf Version: v1.0.0 Editor: @Kevin-Mattheus-Moerman Reviewer: @mgalloy, @leios Archive: 10.5281/zenodo.1256950

Status

status

Status badge code:

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

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

@mgalloy & @leios, 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.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @Kevin-Mattheus-Moerman know.

Review checklist for @mgalloy

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @leios

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 6 years ago

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

  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
whedon commented 6 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 6 years ago

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

Kevin-Mattheus-Moerman commented 6 years ago

@siravan this is where the review will take place. @mgalloy, @leios thanks for acting as reviewers for this submission. There are tickboxes at the top here to guide you through the review process.

Let me know if I can help. :rocket: :robot:

mgalloy commented 6 years ago

I believe the DOI for the "Reconstruction of the action potential of ventricular myocardial fibres" article in your References is "https://doi.org/10.1113/jphysiol.1977.sp011853". I don't see a DOI for the TensorFlow reference.

shahriariravanian commented 6 years ago

Thanks for the comments. The shebangs are fixed. I also added the doi you mentioned. The TensorFlow whitepaper does not seem to have a doi, as it is posted on the TensorFlow website.

mgalloy commented 6 years ago

@siravan A couple questions:

shahriariravanian commented 6 years ago

Yes, I plan to make a release by the end of the review process, when things are more stable.

Thanks for pointing to the lack of contributing guidelines in the README. I will add one soon.

I welcome contributions from a diverse background, as the man goal of fib-tf is experimentation with different ideas regarding solving ODEs with TensorFlow.

Kevin-Mattheus-Moerman commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 6 years ago

PDF failed to compile for issue #719 with the following error:

% Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed

0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 100 13 0 13 0 0 90 0 --:--:-- --:--:-- --:--:-- 90 Error reading bibliography ./paper.bib (line 68, column 1): unexpected "i" expecting space, ",", white space or "}" Error running filter pandoc-citeproc: Filter returned error status 1 Looks like we failed to compile the PDF

Kevin-Mattheus-Moerman commented 6 years ago

@siravan perhaps when you pasted in the DOI you deleted a comma? Not sure if this is it but merge my pull request and we can try @whedon generate pdf again, btw you can ask whedon to do this too.

shahriariravanian commented 6 years ago

Yes, I missed a comma. It is fixed now.

Kevin-Mattheus-Moerman commented 6 years ago

@siravan, in terms of community guidelines and contributing. I would recommend adding a CONTRIBUTING.md file and also a CODE_OF_CONDUCT.md file. You can base the latter on the Contributor Covenant, e.g. version 1.4, available at http://contributor-covenant.org/version/1/4

For more information about community guidelines: https://help.github.com/articles/github-community-guidelines/

Also this link offers example CONTRIBUTING.md files from other projects: https://help.github.com/articles/setting-guidelines-for-repository-contributors/

Let me know if you have questions.

Kevin-Mattheus-Moerman 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: <--

Kevin-Mattheus-Moerman commented 6 years ago

@siravan I see you already added a CONTRIBUTING.md file. I recommend adding a code of conduct file as well.

Kevin-Mattheus-Moerman commented 6 years ago

@mgalloy, @leios can you summarize where we stand with the review process? Thanks

shahriariravanian commented 6 years ago

A CODE_OF_CONDUCT.md file is added to the master branch.

mgalloy commented 6 years ago

@Kevin-Mattheus-Moerman OK, assuming@siravan makes a GitHub release labeled 1.0, then the only thing I haven't found yet is "Functionality documentation", i.e., API documentation. Am I missing it somewhere @siravan?

leios commented 6 years ago

@Kevin-Mattheus-Moerman I apologize for taking so long with this review. I ran into a rather annoying build error when attempting to build tensorflow with JIT and CUDA (https://github.com/tensorflow/tensorflow/issues/10220). It took a while for me to find the solution. I should have been quicker to realize that I needed to use another version of gcc.

@mgalloy The documentation can be found in the docs/ directory under details.html. It's pretty good. There is also a small link under the Documentation heading in the README. It's not exactly obvious to find.

Ultimately, the software works as described; however (@siravan), I have a few lingering questions that have not been addressed here:

  1. The statement of need wasn't quite clear for me. What are the future applications of this software and where has it already been used in an academic (or non-academic) setting? There was a mention of the systems it can be used for, but no mention of what users would be interested in using this software in-particular or any publications that may result from this software. Is it meant to be used by researchers studying stiff ODE systems or is it a test-case to show that tensorflow can provide decently optimized solutions to such systems?

  2. There is a mention of "hand-optimized CUDA code" a lot, but I don't see the code anywhere? I am not sure what you are benchmarking against to get "within a factor of 2-3 of..."

  3. So far as I can tell, this software uses the forward Euler method, which is known to be unstable and carry a high degree of error for stiff ODE's, which is the explicit purpose of this work. Because of this, the forward Euler method is not typically used in too many physics applications. Instead, most researchers prefer using Runge-Kutta, Crank-Nicholson, or pseudo-spectral methods. I understand that this application is primarily for electrophysiological systems, but it would seem like a good idea to include other methods to solve the ODE systems to be safe. If it is commonplace to use more error-prone methods for these types of simulations, then maybe it is fine not to address this point; however, I know that there are researchers who will not use anything with an error of less than dt^3.

  4. Some of the links in the documentation do not work. Those should probably be fixed.

Please let me know if I missed anything!

shahriariravanian commented 6 years ago

@leios Thanks for the comments. I appreciate your comments and persistence in installing the JIT enabled TensorFlow.

  1. The initial reason to develop fib_tf was as a test bed for new ideas on cardiac electrophysiology simulation, like adding a new ionic current to a cardiac model. This is actually how we use it in our academic work (no publication yet, but we are working on a paper on enhancing the Courtemanche atrial model). It is much easier to test ideas in a scripting language like python with all its scientific computing ecosystem than in C++. However, fit_tf can also be useful in assessing the feasibility of writing ODE solvers in TensorFlow.

The main power of TensorFlow is its ability to run on multiple GPUs or even on a distributed system with multiple CPUs, each with one or more GPUs. Writing a C++ code to do this is of course extremely difficult. In this situation, the python code could potentially be faster than the single GPU C++.

  1. The "hand-optimized CUDA code" mentioned here is the production code that we use for 3D cardiac modeling (for example). My plan is to refactor the C++ code and CUDA kernles and add the relevant parts to fib_tf. The C++ code base in its current form is too complex and has lots of functionality unrelated to fib_tf.

  2. Electrophysiology applications are rather unusual in this regard and are commonly solved using the explicit Euler model (or by implicit solvers on occasion). The reason is the extreme stiffness of models. Higher order explicit solvers, such as Runge-Kutta or Linear Multi-Step methods, are not of much help and one still needs a very small time-step for stability. However, the Rush_larsen technique that we have used here is very helpful and allows us to integrate more than half the state variables (the gating variables) by a closed-form integration formula.

Also please note that TensorFlow has an ODE solver (tf.contrib.integrate.odeint), which implements 5th order Runge-Kutta with adaptive step size control and dense output, using the Dormand-Prince method. If it was logical to use a higher order method, we could have used this functionality.

It should be also noted that TensorFlow has an automatic differentiation capability (needed for the machine learning applications). It is possible to implement high order Taylor series methods by using the automatic differentiation (TensorFlow only has backward differentiation, but there is a trick when one can combine two backward differentiations to obtain a forward one needed in the Taylor series). Again this is not a useful technique for cardiac electrophysiology applications, but is potentially applicable to non-stiff models.

  1. The links are fixed.
leios commented 6 years ago

@siravan Thanks for detailed response! I believe that answers all my questions for now, but will take one final look at the code soon.

Kevin-Mattheus-Moerman commented 6 years ago

@mgalloy @leios thanks for your review!! :rocket: can you report on any remaining issues or advise on whether you deem this submission acceptable? @leios the Functionality documentation box is unticked. Can you summarize what you feel is missing? Thanks

leios commented 6 years ago

@Kevin-Mattheus-Moerman All my boxes are ticked and I think this submission is acceptable.

@mgalloy is still missing the functionality documentation box, though. I think they had trouble finding the documentation, so I tried to show them where it was. I don't know if they have looked at this thread since then.

mgalloy commented 6 years ago

The documentation pointed out by @leios is a great explanation . I have checked off the functionality documentation and agree this submission is acceptable.

Kevin-Mattheus-Moerman 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: <--

Kevin-Mattheus-Moerman commented 6 years ago

@siravan looks like we are nearly there. I suggest a minor change to the first sentence in your paper. Instead of on the top of the consider saying on top of the. If you agree with this make the change and render a new pdf here using @whedon generate pdf. Once you are done with that step please archive a copy of the reviewed software using a service like Zenodo (see here to automate DOI creation on new releases), or figshare, and list the DOI of the archived version here.

Kevin-Mattheus-Moerman commented 6 years ago

@leios @mgalloy thanks for the excellent and smooth review process! :tada: :rocket: :robot:

shahriariravanian commented 6 years ago

Thanks a lot for all the help and comments. The paper.md is edited and the repository is archived by Zenodo. DOI is DOI

Kevin-Mattheus-Moerman 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: <--

Kevin-Mattheus-Moerman commented 6 years ago

Great. Thanks @siravan. @arfon we are all set to accept here.

arfon commented 6 years ago

@whedon set 10.5281/zenodo.1256950 as archive

whedon commented 6 years ago

OK. 10.5281/zenodo.1256950 is the archive.

arfon commented 6 years ago

@mgalloy, @leios - many thanks for your reviews here and to @Kevin-Mattheus-Moerman for editing this submission ✨

@siravan - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00719 :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.00719/status.svg)](https://doi.org/10.21105/joss.00719)

This is how it will look in your documentation:

DOI

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: