search

openjournals / joss-reviews

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

[REVIEW]: Flint: a simulator for biological and physiological models in ordinary and stochastic differential equations #2331

Closed whedon closed 3 years ago

whedon commented 4 years ago

Submitting author: @tabe (Takeshi Abe) Repository: https://github.com/flintproject/Flint Version: Flint-2.4.0RC1-JOSS Editor: @majensen Reviewers: @mstimberg, @dawbarton Archive: 10.5281/zenodo.4017040

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

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

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

@mstimberg & @dawbarton, 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 @majensen know.

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

Review checklist for @dawbarton

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @mstimberg

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. @funasoul, @mstimberg it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

: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

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

majensen commented 4 years ago

@whedon remind @majensen in 5 days

whedon commented 4 years ago

Reminder set for @majensen in 5 days

majensen commented 4 years ago

@arfon - should I remind @whedon to remind me? 😄

arfon commented 4 years ago

:-) we'll see...

mstimberg commented 4 years ago

I've started reviewing but I have to say that I had some difficulties to get things going. I'm on Ubuntu, so none of the provided binary packages applies to my situation. I had a look into installing from source, but it seems to require many dependencies that are non-standard and would have to be installed from source as well. Since I did not want to go down that rabbit hole I installed CentOS 8 in a Virtual Machine and used the provided binaries instead. I can confirm that the installation works, but then again things get a bit difficult: the only concrete example file mentioned in the user documentation (I did not look at the Youtube videos, though) is a file "distributed as part of the PhysioDesigner installation". PhysioDesigner seems to be a related project, but it is not mentioned in the JOSS article. The Flint website has a link to www.physiodesigner.org, but this website seems to be down. Same for https://physiome.jp, the other link to an external source on the website (without any explanation what it refers to). I finally got the simulator running with an SMBL file from elsewhere and I will continue to do the review, but as first suggestions for improvement @tabe:

tabe commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

tabe commented 4 years ago

Dear @mstimberg,

* Give some details about related projects such as PhysioDesigner or  physiome.jp in the JOSS article

I've mentioned the physiome.jp project and related tools in the last revision of paper.md. Confirmed their sites are out of reach now, I'm not sure when they will come back though.

* Provide clear instructions how to get example files in the documentation, or distribute them with the software itself

I'm going to reconsider the examples in the user guide, and include the model files in the distribution, too.

* The website has packages for CentOS/RHEL and the github repository also mentions them, but the user guide only has instructions for Windows/OS X

I will update the user guide in order to include descriptions specific to CentOS/RHEL. Thank you for your suggestions.

mstimberg commented 4 years ago

Thanks for the changes so far, I was now able to install Flint from source on Ubuntu 18.04. I opened two issues in the project's repository:

Here a few general comments about the article, the software, and the website:

Minor issues:

tabe commented 4 years ago

Dear @mstimberg,

* Installation instructions: [flintproject/Flint#15](https://github.com/flintproject/Flint/issues/15)

* User manual: [flintproject/Flint#16](https://github.com/flintproject/Flint/issues/16)

Thank you for opening the above issues, we've started working to solve them.

Here a few general comments about the article, the software, and the website:

They are all reasonable, so revising our paper to amend the weakness you pointed out.

* The README mentions that the project's website contains "its user manual and tutorials", but the ["Tutorial videos on YouTube"](https://www.youtube.com/user/PhysioDesigner) linked on that site seem to be tutorials for `PhysioDesigner`, not for `Flint` (at least judging from their titles).

The videos do include how to simulate PHML models by Flint while the focus is how to edit them with PhysioDesigner. We will prepare a new tutorial dedicated to Flint soon.

tabe commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

mstimberg commented 4 years ago

Dear @tabe: given that you left the issues open, I assume you are still working on them? Let me/us know when you want us to have another look.

I am afraid I will be out of office from July 13th to July 31st, though, so I cannot guarantee any timely response during that period.

majensen commented 4 years ago

@mstimberg, just want to thank you for your work so far. @funasoul -- have you had an opportunity to take a look at this software? Thanks very much -

tabe commented 4 years ago

@mstimberg,

Dear @tabe: given that you left the issues open, I assume you are still working on them? Let me/us know when you want us to have another look.

Yes, I will add a comment on each of the issues once it is done, to let you know.

majensen commented 4 years ago

Hello @dawbarton - I know you are engaged in another JOSS review at the moment, but I wondered if this work would be interesting to you. We could use your assistance! Thanks for your consideration, and any suggestions for alternative reviewers. --@majensen

dawbarton commented 4 years ago

Hello @dawbarton - I know you are engaged in another JOSS review at the moment, but I wondered if this work would be interesting to you. We could use your assistance! Thanks for your consideration, and any suggestions for alternative reviewers. --@majensen

I'd be happy to take a look at this but realistically I won't get chance to take a look until w/c 8 August (or slightly after depending on what is happening in that week). If that's fine with you, I'll do it.

majensen commented 4 years ago

Thanks @dawbarton that would be great.

majensen commented 4 years ago

@whedon add @dawbarton as reviewer

whedon commented 4 years ago

OK, @dawbarton is now a reviewer

majensen commented 4 years ago

Added a checklist for you in the header @dawbarton - thanks again

majensen commented 4 years ago

Hi @dawbarton - just following up; are you still able to review as you anticipated? Thanks

dawbarton commented 4 years ago

Hi @dawbarton - just following up; are you still able to review as you anticipated? Thanks

Yes, I can do it. Hope to take a look tomorrow or Friday; early next week at the latest.

dawbarton commented 4 years ago

Installation: I like the list of dependencies for different Linux distributions. The one dependency that isn't listed is gnuplot - it might be worth adding that to the list given that it is required for viewing the plots. I built the package from source under Linux using the supplied Makefile and it was a smooth process.

A statement of need: I can't see any sort of statement of need, comparison with other software, or intended audience in the documentation. There is a clear description of what the software does though. (And I can infer from that who the intended audience is, but it would be good to express it more clearly.) Simply taking some of the text from the JOSS paper and putting in an appropriate place in the documentation would suffice.

Community guidelines: contributions are clearly solicited but there are no guidelines as such just a very brief "Patches and/or suggestions are welcome." It would be good to have a more complete set of guidelines (they don't need to be long, but more than one sentence).

Automated tests: given that this is GUI software, I was pleased to see some automated tests :)

Documentation: a general comment on the documentation is that it would be good to have it as HTML as well as a PDF for accessibility reasons. (This isn't a requirement for JOSS as far as I know, just my personal opinion so feel free to ignore.)

I've attached some comments on the paper; nothing major, though I wasn't very keen on the parameter search section since it makes it sound like a novel algorithm whereas it's really just an exhaustive search (with some bounding applied when it is clear that the current minimum is exceeded). Flint.10.21105.joss.02331.pdf

Overall, this is a nice piece of software that's easy to use. 👍

tabe commented 4 years ago

Dear @dawbarton,

Thank you very much for your constructive comments.

Installation: I like the list of dependencies for different Linux distributions. The one dependency that isn't listed is gnuplot - it might be worth adding that to the list given that it is required for viewing the plots. I built the package from source under Linux using the supplied Makefile and it was a smooth process.

Added sentences about requiring gnuplot in INSTALL.org and PORTABILITY.org.

A statement of need: I can't see any sort of statement of need, comparison with other software, or intended audience in the documentation. There is a clear description of what the software does though. (And I can infer from that who the intended audience is, but it would be good to express it more clearly.) Simply taking some of the text from the JOSS paper and putting in an appropriate place in the documentation would suffice.

Improved the introductory part in README and the user guide.

Community guidelines: contributions are clearly solicited but there are no guidelines as such just a very brief "Patches and/or suggestions are welcome." It would be good to have a more complete set of guidelines (they don't need to be long, but more than one sentence).

We will publish a community guidelines soon. I've filed an issue to work: https://github.com/flintproject/Flint/issues/21

Automated tests: given that this is GUI software, I was pleased to see some automated tests :)

So far we've wondered automated tests through wxWidgets' wxUIActionSimulator, but there is no specific roadmap yet. Filed as a to-do: https://github.com/flintproject/Flint/issues/20

Documentation: a general comment on the documentation is that it would be good to have it as HTML as well as a PDF for accessibility reasons. (This isn't a requirement for JOSS as far as I know, just my personal opinion so feel free to ignore.)

Agreed, we will publish the user guide in HTML at Flint's site. Filed as another to-do: https://github.com/flintproject/Flint/issues/19.

I've attached some comments on the paper; nothing major, though I wasn't very keen on the parameter search section since it makes it sound like a novel algorithm whereas it's really just an exhaustive search (with some bounding applied when it is clear that the current minimum is exceeded). Flint.10.21105.joss.02331.pdf

Updated the draft according to your comments, which were all reasonable.

In Algorithm 1, we say "t found in D" instead of "t \in D" because D is not just a set of time points, but an ordered set of pairs <t, v> where v is the value at time t. While admitting that Algorithm 1 is a simple extention of an exhaustive search, we keep it exposited in our paper for a couple of reasons: (1) it helps understand the timing (in the sense of simulation time) when each thread breaks prematurely, and (2) it clarifies that the necessary resource to be shared in parallel execution is only a double floating-point number with its mutex, which means the overhead is marginal. If an analogy is allowed, it's like Knuth's Algorithm T for a sequential search. It's just a sequential search, but still worth mentioning. Anyway it would be nice to find and cite a reference analyzing the same bounding technique with Algorithm 1 if any.

@whedon generate pdf

mstimberg commented 4 years ago

@tabe Thank you for all the changes to the instructions, user manual and article, I think they improved a lot. I agree with @dawbarton that there should be some minimal form of community guidelines (where to submit bugs, etc.), and this is the only real blocker to acceptance from my side.

majensen commented 4 years ago

Thanks @dawbarton and @mstimberg for all your work on this submission.

@funasoul - I'm assuming that you are not able to review. I will unassign you at this time. If there are any issues with this, please contact me directly at maj -dot- fortinbras -at- gmail -dot- com.

majensen commented 4 years ago

@whedon remove @funasoul as reviewer

whedon commented 4 years ago

OK, @funasoul is no longer a reviewer

dawbarton commented 4 years ago

Everything is good from my perspective, I've checked off all the items on the checklist. I've made a few comments on the community guidelines in https://github.com/flintproject/Flint/commit/9029aecc7433040f405c5db501b2f6686335f150 but they are pretty minor.

mstimberg commented 3 years ago

Everything is good from my side as well, happy to recommend the paper's acceptance in its current form. A very minor final comment on the CONTRIBUTING.md document: In the end it says "Optionally, subscribe the official mailing list flint-discuss@googlegroups.com", but it's not entirely clear how to subscribe (send "subscribe" to that address?). Maybe it could link instead (or additionally) to the google groups website: https://groups.google.com/g/flint-discuss

tabe commented 3 years ago

Maybe it could link instead (or additionally) to the google groups website: https://groups.google.com/g/flint-discuss

All right, done at https://github.com/flintproject/Flint/commit/74cd69a5b6f5abde578901240ac2cc7519971b89 Thanks @mstimberg

majensen commented 3 years ago

Wonderful all, I will do a light proofread of the paper and will get back to you @tabe within a day or two

majensen commented 3 years ago

@whedon check references

majensen commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

/app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:in block in find': No such file or directory - tmp/2331 (Errno::ENOENT) from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:incollect!' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:in find' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-132474d2711b/lib/whedon/processor.rb:61:infind_paper_paths' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-132474d2711b/bin/whedon:50:in prepare' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/command.rb:27:inrun' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:in invoke_command' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor.rb:387:indispatch' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/base.rb:466:in start' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-132474d2711b/bin/whedon:119:in<top (required)>' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in load' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in

'

majensen commented 3 years ago

That's weird (@arfon), didn't compile (in https://github.com/openjournals/joss-reviews/issues/2331#issuecomment-686473971) Was able to create proof using https://whedon.theoj.org/preview, pdf attached.

wyc0zmno62jsio38fcxy.pdf

arfon commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

arfon commented 3 years ago

Sometimes Whedon is a little flaky...

majensen commented 3 years ago

@whedon check references

majensen commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.3389/fphys.2015.00251 is OK
- 10.14326/abe.3.50 is OK
- 10.1109/SAINT.2012.40 is OK
- 10.1016/j.pbiomolbio.2004.01.004 is OK
- 10.1093/bioinformatics/btg015 is OK
- 10.3389/fphys.2010.00164 is OK
- 10.1093/bioinformatics/btl485 is OK
- 10.1126/science.290.5495.1358 is OK
- 10.1073/pnas.0406841102 is OK
- 10.1016/j.imr.2015.12.004 is OK

MISSING DOIs

- None

INVALID DOIs

- None
majensen commented 3 years ago

@tabe - I am going to make one more editing pass, to get the language sounding great. Expect only one more pull request. Thanks!

majensen commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

majensen commented 3 years ago

@tabe we are ready to move forward. Can I ask you to create an archive of the Flint repository (using Zenodo, FigShare, or similar)? The title of your archive should match the title of the paper. Then please report back the DOI of the archive in this thread. Please let me know if you have any questions -