[REVIEW]: Gillespie.jl: Stochastic Simulation Algorithm in Julia

whedon commented 8 years ago

Submitting author: @sdwfrost (Simon Frost) Repository: http://github.com/sdwfrost/Gillespie.jl Version: v0.0.1 Editor: @arfon Reviewer: @vchuravy
Archive: 10.5281/zenodo.59127

Status

Status badge code:

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

Reviewer questions

Conflict of interest

[x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (such as being a major contributor to the software).
General checks
[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Version: Does the release version given match the GitHub release (v0.0.1)?
Functionality
[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: Have any performance claims of the software been confirmed?
Documentation
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g. API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support
Software paper

Paper PDF: 10.21105.joss.00042.pdf

[x] Authors: Does the paper.md file include a list of authors with their affiliations?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] References: Do all archival references that should have a DOI list one (e.g. papers, datasets, software)?

vchuravy commented 8 years ago

@sdwfrost I would recommend to register your package to simplify installation and to tag your package.

@whedon I would offer to review this. How would I proceed?

arfon commented 8 years ago

@whedon I would offer to review this. How would I proceed?

@vchuravy - great! I've just added you to the JOSS reviewers team. Please do the following before starting this review:

Accept the invite to join the 'joss-reviewers' team on GitHub that you should have received.
Familiarize yourself with the JOSS reviewer guidelines
Complete this request for information about your background: https://github.com/openjournals/joss/issues/138

Any questions, please don't hesitate to get in touch.

vchuravy commented 8 years ago

Ok, done.

vchuravy commented 8 years ago

@sdwfrost I will add my thoughts here:

[x] Code-coverage seems not to be working
[x] Performance: Could you add some performance numbers comparing the C and R version with the Julia version.
[x] Tests: The tests you are using are testing functionality. Would it be possible to add correctness tests?
[x] Documentation: It is great to see that you have inline docstrings. Maybe you could use them to generate a documentation (with Documenter.jl)
[x] Please add DOIs to the .bib file

pjotrp commented 8 years ago

This software consists of 100+ lines of Julia code and is a reimplementation of existing code. I would like to ask the author to confirm here that he thinks this software is a valuable contribution to research. I want to point out that once published this work will be visible for a long time. Adding an example of having used this tool in research would be a good idea.

Also a speed claim should show numbers and a version of 0.01 looks alpha to me.

sdwfrost commented 8 years ago

@vchuravy Thanks for the feedback!

The package is on its way to being in METADATA.jl (overlooked version number for Julia in requires)
Coverage is now fixed (silly error) - at 93%.
Happy to put in the comparisons. Would R and Rcpp suffice?
As these are stochastic simulations, it's a bit hard to check. I can test for consistency (such that examples give fixed outputs).
I haven't used Documenter.jl. Will give it a go.
Will add DOIs.

@pjotrp Thanks for the feedback too!

There aren't many tools out there that allow one to easily run these types of simulations - at least, those that aren't heavily based on SBML. The paper for GillespieSSA only has 28 citations on Google Scholar - not great for a tool - but other packages e.g. hybridModels use it, and other packages (e.g. POMP) have their own custom implementation. The overall style of the package (and Julia-specific ways of speeding things up), was originally borrowed by @rveltz, and we're now working on implementations of more research-level algorithms. So, despite being a small library (it'd be longer in Java ;) ), on balance, I say that it's worth it.
The version number is following Julia guidelines (as Julia is < 1.0 right now), rather than necessarily reflecting an alpha stage.

I also see the 'Community' checkbox is unchecked. I'll add a section there too.

Thanks again @vchuravy @pjotrp @arfon @whedon I think that this is a great initiative.

rveltz commented 8 years ago

Dear Simon,

I don’t understand the meaning of this mail. Is it some reviews from joss for Gillespie.jl?

Romain

On 21 Jul 2016, at 12:27, Simon Frost notifications@github.com wrote:

@vchuravy https://github.com/vchuravy Thanks for the feedback!

The package is on its way to being in METADATA.jl (overlooked version number for Julia in requires) Coverage is now fixed (silly error) - at 93% https://coveralls.io/github/sdwfrost/Gillespie.jl?branch=master. Happy to put in the comparisons. Would R and Rcpp suffice? As these are stochastic simulations, it's a bit hard to check. I can test for consistency (such that examples give fixed outputs). I haven't used Documenter.jl. Will give it a go. Will add DOIs. @pjotrp https://github.com/pjotrp Thanks for the feedback too!

There aren't many tools out there that allow one to easily run these types of simulations - at least, those that aren't heavily based on SBML. The paper for GillespieSSA only has 28 citations on Google Scholar - not great for a tool - but other packages e.g. hybridModels use it, and other packages (e.g. POMP) have their own custom implementation. The overall style of the package (and Julia-specific ways of speeding things up), was originally borrowed by @rveltz https://github.com/rveltz, and we're now working on implementations of more research-level algorithms. So, despite being a small library (it'd be longer in Java ;) ), on balance, I say that it's worth it. The version number is following Julia guidelines (as Julia is < 1.0 right now), rather than necessarily reflecting an alpha stage. I also see the 'Community' checkbox is unchecked. I'll add a section there too.

Thanks again @vchuravy https://github.com/vchuravy @pjotrp https://github.com/pjotrp @arfon https://github.com/arfon @whedon https://github.com/whedon I think that this is a great initiative.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/42#issuecomment-234215995, or mute the thread https://github.com/notifications/unsubscribe-auth/ABhKBCdjDAN2EOiFA3-3-LPlszJ5rThkks5qX0mFgaJpZM4JRJlz.

sdwfrost commented 8 years ago

Hi @rveltz - I just mentioned you in conversation in my responses to referees (as it says on the bottom of the email) - this also gives you the option to mute the thread.

rveltz commented 8 years ago

Hi,

I did not know you could share reviews like that,

Romain

On 21 Jul 2016, at 13:34, Simon Frost notifications@github.com wrote:

Hi @rveltz https://github.com/rveltz - I just mentioned you in conversation in my responses to referees (as it says on the bottom of the email) - this also gives you the option to mute the thread.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/42#issuecomment-234228603, or mute the thread https://github.com/notifications/unsubscribe-auth/ABhKBH4VRpSPbXeAXPX1PsDUqrpgbs1Fks5qX1lFgaJpZM4JRJlz.

arfon commented 8 years ago

Thanks for review @vchuravy and the updates @sdwfrost. @sdwfrost please let me know when you've had a chance to complete the other items.

vchuravy commented 8 years ago

@sdwfrost Thank for including the benchmarks. Just for your information @benchmark takes the parameters samples and seconds as limits. So in your case the benchmark is not running for a 1000 samples because the time limit is exceeded, to turn that of you can set seconds=Inf. Also it is preferential to write $x0 instead of x0, otherwise you will also benchmark the cost of accessing a global variable (expensive in Julia).

@benchmark ssa(x0,F,nu,parms,tf) samples=1000
# should be
@benchmark ssa($x0,$F,$nu,$params,$tf) samples=1000 seconds=Inf

Also the times for R,Rccp,and SSA should probably be divided by N so that they are more readily comparable.

sdwfrost commented 8 years ago

@vchuravy I didn't get round to finishing the writeup on the benchmarks; thanks for the tips on @benchmark; I didn't know that you could use interpolation. Working through Documenter.jl right now. Will update here when the rest of the items are complete. Thanks again for your input.

sdwfrost commented 8 years ago

@vchuravy I've added some documentation via Documenter.jl - is this OK? - and updated the README to include proposed enhancements. I'm working on updating the benchmarks.

sdwfrost commented 8 years ago

Dear @vchuravy @arfon @pjotrp

I think I've addressed all your comments now. Please let me know if there is anything I have missed. In summary

Documentation is written using Documenter.jl
Updated benchmarks are included for Julia and R/Rcpp. Although the plot doesn't show up in the IJulia notebook, the output is comparable between a handcoded version and Gillespie.jl. These benchmarks are summarised in the README.md. I hope that these results provide sufficient rationale for inclusion of Gillespie.jl in JOSS. I have also included a benchmark file in the repository.
The tests now include checks that the output is at fixed for a given random number seed (in addition to checking that all the examples run). Code coverage is 95%
I've added some material to the end of the README with respect to future directions etc.
dois added to the bib file.
Release v0.0.1 added to the repository (rationale for numbering is following the Julia convention).
Various badges have been added, including links to stable and latest docs, Coveralls.io, Waffle.io, and JOSS.

Thanks!

vchuravy commented 8 years ago

@arfon How would one regenerate the pdf? The .bib now includes DOI's, but they don't show up the the pdf. @sdwfrost Great work! The codecoverage seems to be off again. One last question from my side. Have you tested compatibility with Julia v0.5 (the rc0 was just announced)?

Otherwise this is a green light from me.

arfon commented 8 years ago

@arfon How would one regenerate the pdf? The .bib now includes DOI's, but they don't show up the the pdf.

@vchuravy here's the updated PDF: 10.21105.joss.00042.pdf

@sdwfrost Great work! The codecoverage seems to be off again. One last question from my side. Have you tested compatibility with Julia v0.5 (the rc0 was just announced)?

@sdwfrost - once you've addressed this comment I think we're good to accept this submission 🎉

sdwfrost commented 8 years ago

@arfon @vchuravy Compatibility now checked against v0.4.6 and v0.5, all tests passing. I get some deprecation warnings on v0.5, but future releases will fix those.

@vchuravy I was getting some funny badges on Chrome for Coveralls (fine on Firefox) - click through, and you'll see coverage is still good.

vchuravy commented 8 years ago

@sdwfrost Perfect thanks.

arfon commented 8 years ago

@sdwfrost - as a final step, could you make an archive of the code in Zenodo/figshare? Please update this thread with the DOI of the code archive.

sdwfrost commented 8 years ago

@arfon DOI from Zenodo: 10.5281/zenodo.59127

arfon commented 8 years ago

Thanks @sdwfrost. Your paper is now accepted into JOSS and your DOI is http://dx.doi.org/10.21105/joss.00042 :rocket: 🎉 💥

Thanks for the review @vchuravy

openjournals / joss-reviews