openjournals / joss-reviews

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

[REVIEW]: graphsim: An R package for simulating gene expression data from graph structures of biological pathways #2161

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @TomKellyGenetics (S. Thomas Kelly) Repository: https://github.com/TomKellyGenetics/graphsim Version: v1.0.0-joss Editor: @majensen Reviewer: @rcannood, @corybrunson Archive: 10.5281/zenodo.3931288

Status

status

Status badge code:

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

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

@rcannood & @corybrunson, 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 two weeks

Review checklist for @rcannood

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @corybrunson

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

TomKellyGenetics commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

TomKellyGenetics commented 4 years ago

Quick update here. I don't have as much time to work on this as I'd like. I will try to get back to vignettes sometime soon.

I've removed the code for the subcaptions from the examples (these run hidden). This makes the Rmarkdown messy but I think it's good to refer to if you wish to reproduce the figures. Regarding length, I get a preview from pandoc/LaTeX of 11 pages (including 2 of bibliography) but the Whedon version is longer (13) due to different figure placement. I've altered the code examples to fit within the margins of the JOSS style. I'll see what I can do about the figures, making them smaller doesn't seem to help. Possibly moving them to different pages would.

majensen commented 4 years ago

@TomKellyGenetics I really like this paper, and the material there is helpful and good. One way to reduce the size might be to take the example section and refer to it by reference from the paper to place where it is rendered -- could be a document in the repo itself, or simply the command to display the vignette in all its glory.

TomKellyGenetics commented 4 years ago

Thanks @majensen and all for the feedback, that is really encouraging. I've begun to revise the vignettes (see https://github.com/TomKellyGenetics/graphsim/commit/84100b88036287aee9a7db1497c2249107132da9 for details). The next step will be to add examples with the empirical datasets provided in data/ (I've done this for the plotting function already https://github.com/TomKellyGenetics/graphsim/commit/faea482b8d9e3a3a0af2a9d791060595ded67051). Once the vignettes are revised, I think they would be suitable to refer (or link) to for detailed (reproducible) examples as you suggest. The full Rmarkdown file will be available in the repository as well.

Sorry for the delays on this. Now that the vignettes have been restructured, hopefully it won't take much longer to complete them and updates the examples in the documentation.

majensen commented 4 years ago

Hi @TomKellyGenetics - how are things progressing?

TomKellyGenetics commented 4 years ago

Hi @majensen, thanks for following up. I've been busy with other projects but I've got another round of revisions almost ready push to GitHub. This should address the remaining issues on the graphsim package repository.

I've reached out to my coauthor (former supervisor) for a second opinion before releasing the updated manuscript. At the moment, I'm making sure the package with updated documentation passes CRAN checks and formatting the manuscript so that it is shorter.

It should be ready for the reviewers to check soon (I think next week).

TomKellyGenetics commented 4 years ago

@majensen @rcannood @corybrunson

Hi everyone,

I'm pleased to update that I've pushed a substantial revision to the package repository. The main changes are to the documentation (examples and vignettes) and the manuscript. Some examples in the manuscript have been removed (but are available in the Rmarkdown version and vignettes. Figure placement by Whedon is hardcoded with \begin{figure}[H] which explains this issues we had before. This makes the PDF several pages shorter as suggested.

I think this addresses all remaining issues raised by the reviewers on the graphsim repository. It is now ready for the reviewers to check again. I understand that this may take some time. Sorry that it has taken me longer than I expected to get to this point. I think the package and paper have improved as a result so thank you all for your feedback.

I've submitted updates to CRAN and biorXiv to reflect these revisions. I look forward to your comments.

Thank you!

TomKellyGenetics commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

TomKellyGenetics commented 4 years ago

@whedon set <v1.0.0> as version

whedon commented 4 years ago

I'm sorry @TomKellyGenetics, I'm afraid I can't do that. That's something only editors are allowed to do.

rcannood commented 4 years ago

Good job @TomKellyGenetics :+1: ! I went through the new versions of the article and the documentation on the graphsim GitHub repo. I'm pleased to see that the article is looking really nice. The changes you made adequately address my previous concerns.

majensen commented 4 years ago

@whedon set v1.0.0 as version

whedon commented 4 years ago

OK. v1.0.0 is the version.

corybrunson commented 4 years ago

@TomKellyGenetics most everything looks good. I've commented on the remaining open issues i raised, and i the package and paper don't suggest to me that you're making performance claims, so my only remaining issue is that the last link in the paper (in the Computational details section) is broken. Instead of linking to the paper folder in the repo, i'd suggest just linking to the repo.

TomKellyGenetics commented 4 years ago

Thanks everyone for the quick and helpful responses. I've updated the manuscript to link to the GitHub repo correctly.

I agree to leaving the remaining issues on the repo open until I've had more time to look at the suggestions for documentation in more detail. I will consider these changes for the next release on CRAN.

TomKellyGenetics commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

corybrunson commented 4 years ago

@TomKellyGenetics that's reasonable! Also, the package installs for me from CRAN, including the vignettes.

@majensen my concerns have been addressed and i have completed my review. : )

majensen commented 4 years ago

@whedon check references

whedon commented 4 years ago
Reference check summary:

OK DOIs

- 10.1093/nar/gkt1102 is OK
- 10.1093/imanum/22.3.329 is OK
- 10.1093/bioinformatics/btm639 is OK
- 10.1186/gb-2014-15-2-r29 is OK
- 10.1101/2020.02.06.936971 is OK
- 10.1101/716811 is OK
- 10.1186/1752-0509-3-41 is OK

MISSING DOIs

- https://doi.org/10.1038/nrg1272 may be missing for title: Network biology: understanding the cell’s functional organization
- https://doi.org/10.1007/978-3-642-01689-9 may be missing for title: Computation of Multivariate Normal and t Probabilities
- https://doi.org/10.3389/fgene.2019.00535 may be missing for title: Inferring Interaction Networks From Multi-Omics Data
- https://doi.org/10.1038/nrg.2016.87 may be missing for title: Network biology concepts in complex disease comorbidities
- https://doi.org/10.3892/ijo.2012.1744 may be missing for title: Molecular features of triple negative breast cancer cells by genome-wide gene expression profiling analysis
- https://doi.org/10.1186/s12859-015-0778-7 may be missing for title: Comparing the normalization methods for the differential analysis of Illumina high-throughput RNA-Seq data
- https://doi.org/10.1186/1471-2105-8-s6-s5 may be missing for title: Inferring cellular networks–a review
- https://doi.org/10.1093/nar/gkv007 may be missing for title: limma powers differential expression analyses for RNA-sequencing and microarray studies
- https://doi.org/10.1038/nprot.2017.149 may be missing for title: Exponential scaling of single-cell RNA-seq in the past decade
- https://doi.org/10.1038/nrg2934 may be missing for title: RNA sequencing: advances, challenges and opportunities
- https://doi.org/10.1101/227033 may be missing for title: Gene expression distribution deconvolution in single-cell RNA sequencing
- https://doi.org/10.1109/msp.2007.273053 may be missing for title: Finding module-based gene networks with state-space models - Mining high-dimensional and short time-course gene expression data
- https://doi.org/10.1186/s13059-017-1305-0 may be missing for title: Splatter: simulation of single-cell RNA sequencing data

INVALID DOIs

- None
majensen commented 4 years ago

@TomKellyGenetics - we're on the final leg of this journey.

TomKellyGenetics commented 4 years ago

@majensen Thanks, that's great to hear! I've updated paper.bib to include the DOIs.

For the "archive" version, is a GitHub "release" okay? I've already tagged version 1.0.0 which was released on CRAN. I've integrated Zenodo with package so a DOI: 10.5281/zenodo.3926914 has already been generated automatically. Is this appropriate or is a dedicated tar.gz with the paper name (as done here) preferable?

TomKellyGenetics commented 4 years ago

A new version has been released on Zenodo DOI 105281/zenodo.3931288 to correspond to the manuscript. This version has zip and tar.gz formats available for all files relevant to the paper and software release. This version is tagged with 1.0..0-joss, the software is the same as the CRAN release with minor tweaks to the paper discussed above. Hope that does the trick.

majensen commented 4 years ago

Should be fine @TomKellyGenetics

majensen commented 4 years ago

I will do a quick proofread today - may engender a PR - just FYI @TomKellyGenetics

majensen commented 4 years ago

@whedon check references

majensen commented 4 years ago

@whedon commands

danielskatz commented 4 years ago

👋 @openjournals/dev - can you take a look at this?

arfon commented 4 years ago

@whedon check references

arfon commented 4 years ago

Looks like there's a typo in the BibTeX here: https://github.com/TomKellyGenetics/graphsim/blob/master/paper/paper.bib#L100 (the period needs removing).

I'm going to look at adding better error handling for the BibTeX parser in Whedon.

majensen commented 4 years ago

@arfon @danielskatz thanks for having a look - @TomKellyGenetics, can you have a look at the bibtex file per https://github.com/openjournals/joss-reviews/issues/2161#issuecomment-654430778 ?

arfon commented 4 years ago

@whedon check references

whedon commented 4 years ago

Checking the BibTeX entries failed with the following error:

Failed to parse BibTeX on value "." (NAME) [#, "@", #, {:author=>["Hawe, J. S., and Theis, F. J., and Heinig, M.,"], :title=>["{{I}nferring {I}nteraction {N}etworks {F}rom {M}ulti-{O}mics {D}ata}"], :journal=>["Front Genet"], :year=>["2019"], :volume=>["10"], :pages=>["535"], :doi=>["10.3389/fgene.2019.00535"]}]

danielskatz commented 4 years ago

the author field appears to be wrong - it should be "Hawe, J. S. and Theis, F. J. and Heinig, M."

arfon commented 4 years ago

Also, there's an issue with doi = {10.3389/fgene.2019.00535}. <- the period needs removing on this line.

TomKellyGenetics commented 4 years ago

@whedon check references

TomKellyGenetics commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

Error reading bibliography ./paper.bib (line 357, column 1): unexpected end of input expecting "\"", "\" or "{" Error running filter pandoc-citeproc: Filter returned error status 1 Looks like we failed to compile the PDF

TomKellyGenetics commented 4 years ago

@whedon generate pdf

TomKellyGenetics commented 4 years ago

@whedon check references

whedon commented 4 years ago

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

sh: 0: getcwd() failed: No such file or directory sh: 0: getcwd() failed: No such file or directory Error producing PDF. ! LaTeX Error: File `Plotsimple_graph-1' not found.

See the LaTeX manual or LaTeX Companion for explanation. Type H for immediate help. ...

l.588 ...eight=.375\linewidth]{Plotsimple_graph-1}

Looks like we failed to compile the PDF

whedon commented 4 years ago
Reference check summary:

OK DOIs

- 10.1038/nrg1272 is OK
- 10.1093/nar/gkt1102 is OK
- 10.1007/978-3-642-01689-9 is OK
- 10.3389/fgene.2019.00535 is OK
- 10.1093/imanum/22.3.329 is OK
- 10.1093/bioinformatics/btm639 is OK
- 10.1038/nrg.2016.87 is OK
- 10.3892/ijo.2012.1744 is OK
- 10.1186/gb-2014-15-2-r29 is OK
- 10.1186/s12859-015-0778-7 is OK
- 10.1186/1471-2105-8-s6-s5 is OK
- 10.1093/nar/gkv007 is OK
- 10.1038/nprot.2017.149 is OK
- 10.1038/nrg2934 is OK
- 10.1101/2020.02.06.936971 is OK
- 10.1101/716811 is OK
- 10.1186/1752-0509-3-41 is OK
- 10.1101/227033 is OK
- 10.1109/msp.2007.273053 is OK
- 10.1186/s13059-017-1305-0 is OK

MISSING DOIs

- None

INVALID DOIs

- None
arfon commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

TomKellyGenetics commented 4 years ago

Typos in references corrected. Just pushed updated PDF of figures. @majensen PRs welcome :-)

majensen commented 4 years ago

Hi @TomKellyGenetics - well, it's a very clearly written paper, and I enjoyed going through it again. The only tiny thing I would have done is remove the second comma in this snippet at (raw) line 137 in paper.md:

studies, as well as, bulk

i.e., "studies, as well as bulk...".

Not worth a PR, IMO. I will see if I can cause the editor-in-chief to manifest him/herself.

majensen commented 4 years ago

@whedon set v1.0.0-joss as version

whedon commented 4 years ago

OK. v1.0.0-joss is the version.