search

openjournals / joss-reviews

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

[REVIEW]: Artery.FE: An implementation of the 1D blood flow equations in FEniCS #1107

Closed whedon closed 5 years ago

whedon commented 5 years ago

Submitting author: @akdiem (Alexandra Diem) Repository: https://github.com/KVSlab/bloodflow Version: v1.0 Editor: @trallard Reviewer: @dlagrava, @jhill1 Archive: 10.5281/zenodo.2383815

Status

status

Status badge code:

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

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

@dlagrava & @jhill1, 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 @trallard know.

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

Review checklist for @dlagrava

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @jhill1

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 5 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @dlagrava, 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 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

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

trallard commented 5 years ago

Hi reviewers and @akdiem ! This is the issue we will be using to conduct the review. I know @jhill1 is away for a couple of weeks but in the meantime, @dlagrava can get started with the review and recommendations (if any).

If any of you encounters any issues / need any guidance during the review process feel free to ping me here 👩🏻‍💻

dlagrava commented 5 years ago

@akdiem I have started this. The first comment that I have is that you actually do not have a release v1.0 on your github. Could you please create one? Thanks.

alexdiem commented 5 years ago

@dlagrava Apologies, I must have forgotten because it is not mentioned in the submission guide. I've done this now.

Edit: Possibly the checklist above could be added to the submission guide on readthedocs to avoid this.

trallard commented 5 years ago

Thanks for the note @akdiem I will revise the submission guide and make this clearer for future submissions

dlagrava commented 5 years ago

@akdiem I have gone through the installation part. It is really a great idea to provide a Docker container. My only comment at this point is that the python command inside the container points to python 2, and you need to specifically need to execute the code with python3. Could you please update the readthedocs to reflect this or make the default python to be 3 on the Dockerfile. Thanks.

dlagrava commented 5 years ago

@akdiem I have tried to run the unit tests that you have provided. The test_artery_network.py does not work as the class it reffers to seems to have changed from "Artery_Network" to "ArteryNetwork". Thanks.

alexdiem commented 5 years ago

@dlagrava Apologies for the oversight. I have fixed both issues. I decided to change the documentation to state using 'python3' as there seems to be no easy way to set the default to Python 3 in Docker.

alexdiem commented 5 years ago

@whedon generate pdf

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

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

alexdiem commented 5 years ago

@whedon generate pdf

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

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

jhill1 commented 5 years ago

Hi @akdiem. I found the package very interesting and nice use of Fenics. The module is very well documented with inline comments, and the documentation has examples, tests. The paper is well structured with a clear aim for the software and the equation laid out are easy to find in the corresponding code. The tests worked out of the box for me and installation was straightforward using the Docker or the more traditional setup.py.

The example given is well described, giving reference to the paper that it uses and the plot produced even use a good colour scheme!

There are a few minor issues with formatting of references in the paper and I would recommend hooking up the tests to TravisCI or similar system for continuous testing. The former should be done before accepting this paper, but the latter is at the discretion of the authors (as there are manually running tests).

Once the issues with the referencing are sorted, this paper should be accepted.

Note to @trallard I'm not an expert in blood flow equations, but the Fenics code looks good to me.

dlagrava commented 5 years ago

Hello @akdiem I have finished my review as well. I performed some blood simulations for stent optimisation on 3D and it was very time consuming, probably a 1D approach should be more efficient, while retaining most of the average behavior. I quite enjoyed the presentation of the theory of the 1D bloodflow equations on your readthedocs, which is a nice touch for the interested reader.

Your implementation based on FEnICS is well documented, the installation and the tests work well. One question would be how you manage/inform about updates and bug fixes on FEnICS.

The last comment that I have is that your readme.md on the project should provide guidelines stating that people are welcome to contribute and submit bugs, and who to ask questions to.

It was an interesting review. I wish you a lot of success.

@trallard Once the comment about community guidelines is fixed, I have finished with the review.

alexdiem commented 5 years ago

@whedon generate pdf

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

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

alexdiem commented 5 years ago

@whedon generate pdf

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

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

alexdiem commented 5 years ago

@jhill1 Thank you for your review and your comments! I agree that CI use would be ideal. It has been quite tricky so far to set up the specific version of Fenics on Travis as I would need to install it via docker. I will look into this again though.

alexdiem commented 5 years ago

@dlagrava Thank you for your review and your comments! I have added a "Support" section to the README, stating the info you requested. If you would like to explore running your simulations using artery.FE, I'd be more than happy to assist. Feel free to contact me about this. Edit: With regards to Fenics updates, I will do my best to keep up with its development and adjust the code when necessary and always state the lowest running version. With docker it is luckily very easy to test different versions of Fenics.

trallard commented 5 years ago

hey @dlagrava thanks for your thorough review, I just noticed that you left the following item unchecked:

Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Is there anything that still needs doing in this area?

@jhill1 thanks for your review and additional comments to make the testing and code more robust

dlagrava commented 5 years ago

@trallard sorry, that was an oversight. It is fixed now. I did finish the review. Thanks a lot!

trallard commented 5 years ago

Hey no worries @dlagrava thanks again for your time

As soon as @jhill1 confirms that the community guidelines point has been satisfactorily added I will conduct the last editorial tasks on the submission

jhill1 commented 5 years ago

Done!

trallard commented 5 years ago

@akdiem now that the reviews are completed and the paper has been recommended for acceptance, we need you to complete the following actions:

alexdiem commented 5 years ago

@whedon generate pdf

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

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

alexdiem commented 5 years ago

@trallard I had forgotten to add the acknowledgements to the paper - I hope it is ok to still add them. I have rebuilt the paper and will perform the other final steps now.

alexdiem commented 5 years ago

@trallard I've completed the tasks, the doi for the new release is https://doi.org/10.5281/zenodo.2383815

trallard commented 5 years ago

@whedon set 10.5281/zenodo.2383815 as archive

whedon commented 5 years ago

OK. 10.5281/zenodo.2383815 is the archive.

trallard commented 5 years ago

@arfon: this submission is accepted and ready to be published 🎉👾

@jhill1 and @dlagrava thank you very much for your time and valuable contribution to JOSS as a reviewer for this submission 🙌🏻🌟

arfon commented 5 years ago

@whedon accept

whedon commented 5 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 5 years ago

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/123

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/123, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
arfon commented 5 years ago

@whedon accept deposit=true

whedon commented 5 years ago 
Doing it live! Attempting automated processing of paper acceptance...
whedon commented 5 years ago

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited :point_right: https://github.com/openjournals/joss-papers/pull/124
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.01107
  3. If everything looks good, then close this review issue.

  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

    Any issues? notify your editorial technical team...

arfon commented 5 years ago

@dlagrava, @jhill1 - many thanks for your reviews here and to @trallard for editing this submission ✨

@akdiem - your paper is now accepted into JOSS :zap::rocket::boom:

whedon commented 5 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 snippets:

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

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.01107">
  <img src="http://joss.theoj.org/papers/10.21105/joss.01107/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: http://joss.theoj.org/papers/10.21105/joss.01107/status.svg
   :target: https://doi.org/10.21105/joss.01107

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: