search

ropensci / software-review

rOpenSci Software Peer Review.
290 stars 104 forks source link

iheatmapr #107

Closed AliciaSchep closed 7 years ago

AliciaSchep commented 7 years ago

Summary

Makes complex, interactive heatmaps. The package includes a modular system for iteratively building up complex heatmaps, as well as the iheatmap function for making relatively standard heatmaps. 

Package: iheatmapr
Type: Package
Title: Interactive Complex Heatmaps
Version: 0.2.4
Authors@R: c(person("Alicia", "Schep", email = "aschep@gmail.com",
                  role = c("aut", "cre")),
              person("Sarah","Kummerfeld", email = "kummerfeld.sarah@gene.com",
                  role = c("aut")),
              person("Genentech", role = "cph"))
Description: iheatmapr is a package for making complex, interactive heatmaps.
    The package includes a modular system for iteratively building up complex
    heatmaps, as well as the iheatmap function for making relatively standard
    heatmaps.
License: MIT + file LICENSE
Depends:
    plotly (>= 4.1.1),
    R (>= 2.10)
Imports:
    methods,
    plyr,
    utils,
    magrittr,
    S4Vectors,
    stats,
    ggdendro,
    fastcluster,
    RColorBrewer,
    htmlwidgets,
    jsonlite,
    scales,
    knitr
Suggests:
    htmltools,
    datasets,
    shiny,
    testthat,
    rmarkdown,
    reshape2,
    roxygen2,
    covr
biocViews: Visualization
RoxygenNote: 6.0.1
VignetteBuilder: knitr
URL: https://github.com/AliciaSchep/iheatmapr
BugReports: https://github.com/AliciaSchep/iheatmapr/issues
Collate:
    'AllClasses.R'
    'AllGenerics.R'
    'annotations.R'
    'as_plotly.R'
    'axes.R'
    'axis_labels.R'
    'axis_titles.R'
    'barplot.R'
    'clustering.R'
    'clusters.R'
    'colorbars.R'
    'colors.R'
    'components.R'
    'dendogram.R'
    'generic_subplots.R'
    'groups.R'
    'main_heatmap.R'
    'iheatmap.R'
    'iheatmapr.R'
    'layout.R'
    'shiny_click_test.R'
    'signal.R'
    'subplot.R'
    'summary.R'
    'utils.R'

https://github.com/AliciaSchep/iheatmapr

Anyone who wants to visualize data using heatmaps. Package is not intended to be domain specific, although some fields tend to use heatmaps more than others.

There are great tools in R for creating relatively simple interactive heatmaps (plotly, d3heatmap, heatmaply) or creating static complex heatmaps (ComplexHeatmap). However, there are no tools (that I am aware of) facilitating easy creation of complex, interactive heatmaps.

Requirements

Confirm each of the following by checking the box. This package:

Publication options

Detail

maelle commented 7 years ago

Editor checks:

Editor comments

Thanks for your submission @AliciaSchep! :grinning: Currently looking for reviewers.

A few comments: 

It is good practice to

  ✖ write unit tests for all functions, and all package code in
    general. 78% of code lines are covered by test cases.

    R/AllGenerics.R:93:NA
    R/AllGenerics.R:96:NA
    R/AllGenerics.R:141:NA
    R/AllGenerics.R:144:NA
    R/AllGenerics.R:201:NA
    ... and 587 more lines

  ✖ not use "Depends" in DESCRIPTION, as it can cause name clashes,
    and poor interaction with other packages. Use "Imports" instead.
  ✖ avoid long code lines, it is bad for readability. Also, many
    people prefer editor windows that are about 80 characters wide.
    Try make your lines shorter than 80 characters

    R\AllGenerics.R:34:1
    R\axes.R:389:1
    R\axes.R:390:1
    R\axes.R:510:1
    R\axes.R:630:1
    ... and 37 more lines

  ✖ not import packages as a whole, as this can cause name clashes
    between the imported packages. Instead, import only the specific
    functions you need.
  ✖ fix this R CMD check NOTE: Namespace in Imports field not
    imported from: 'fastcluster' All declared Imports should be
    used.
  ✖ fix this R CMD check NOTE: add_col_clustering,Iheatmap: no
    visible global function definition for 'hclust'
    add_row_clustering,Iheatmap: no visible global function
    definition for 'hclust' Undefined global functions or variables:
    hclust Consider adding importFrom("stats", "hclust") to your
    NAMESPACE file.

Reviewers: @andeek @carlganz Due date: 2017-04-23

AliciaSchep commented 7 years ago

Thanks @maelle!

I have updated the typos, long lines, and some of the NAMESPACE issues.

For the use of DEPENDS, I think it is appropriate in this case to force user import of plotly. For importing the packages as whole, I am only importing S4Vectors and methods -- as I am using a lot of functions & classes from these packages I thought that might be appropriate, but am of course open to more guidance on this question.

I will work on adding more tests to get closer to 100% test coverage.

maelle commented 7 years ago

Great but note that 100% coverage isn't compulsory 😉

maelle commented 7 years ago

@AliciaSchep the reviewers are now assigned! 😀

Thanks for agreeing to review this package @andeek @carlganz! 😃

As a reminder here are links to the recently updated reviewing and packaging guides and to the review template.

carlganz commented 7 years ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

Paper (for packages co-submitting to JOSS)

The package contains a paper.md with:

  • [X] A short summary describing the high-level functionality of the software
  • [X] Authors: A list of authors with their affiliations
  • [X] A statement of need clearly stating problems the software is designed to solve and its target audience.
  • [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

Final approval (post-review)

Estimated hours spent reviewing: 3

Review Comments

This package provides R users with a collection of easy-to-use functions for iteratively building complex interactive heatmaps. The iheatmap function initiates a heatmap, and a variety of function of the form add_* allow thee user to include annotations, barplots, clustering, grouping, dendrograms, labels, plots, titles, and summaries to both the rows, and columns as well as additional heatmaps. The package makes nice use of magrittr pipes, and plotly graphics, which gives it a nice modern feel.

The package is extremely well documented with an extensive vignette, and examples for almost every function. The package is also well tested with 79% code coverage. It contains a library of graphs, which are recreated, and then compared to in the tests.

Overall the package is very well done, and meets all the rOpenSci packaging guidelines. Functions have proper names, and documentation. Use of plotly in depends makes sense.

A few suggestions:

Hope that helps.

Kind Regards, Carl

maelle commented 7 years ago

Thanks a lot for your review @carlganz !

AliciaSchep commented 7 years ago

@carlganz thank you for the review and helpful suggestions! I will follow up on all the suggestions and post an update to this thread when that is complete.

andeek commented 7 years ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

Paper (for packages co-submitting to JOSS)

The package contains a paper.md with:

Functionality

Estimated hours spent reviewing: 3

Review Comments

Overall, this package is really great. I had a really fun time playing with and and definitely see the importance of the work. I think the modular structure fits really nicely with how R is trending lately (in terms of piping) and I appreciate that that must not have been super straightforward to create. Overall, I think is a wonderful contribution and is very fun as well!

In addition, the vignette was extremely well put together and I appreciate the thorough documentation.

I have a few suggestions and explanations for any tasks above that did not receive an "X".

Explanations for no "X"s above and suggestions for improvement

Perhaps you could explain the difference between add_col_clustering() and add_col_dendrogram(), it looks like they are very similar in the result.

Documentation
Paper (for packages co-submitting to JOSS)
Functionality
AliciaSchep commented 7 years ago

Thanks @andeek for the review! I hope to have time soon to address all issues in both reviews.

maelle commented 7 years ago

Thanks a lot @andeek ! :smile_cat:

@AliciaSchep now the two reviews are in, let us know if you have any question!

maelle commented 7 years ago

@AliciaSchep I see you've done some work on the package, do you think you'll soon be able to answer to the reviews? :wink:

AliciaSchep commented 7 years ago

Sorry, I'm behind on everything! I have made most of the changes suggested, although I still need to figure out why the tests on Appveyor are failing...

maelle commented 7 years ago

No problem! I work on Windows, so if you need me to test anything, just ask 😉

AliciaSchep commented 7 years ago

Thanks @maelle, I may finally have figured it out -- I think it is a question of precision as the tests seem to pass with 64 bit architecture but not 32 bit (updating appveyor to use 64 bit led to tests passing)... I'll need to change the tests a bit to not be so sensitive to precision (no good reason for the test data to have so many significant digits anyways)

maelle commented 7 years ago

Oh cool, nice catch! :fishing_pole_and_fish:

AliciaSchep commented 7 years ago

I have now addressed almost all of the reviewer concerns, thanks again to @andeek and @carlganz for taking the time to review the package.

Most of the references are other packages, and I couldn't find DOI, but I have added the DOI for the reference that is a paper.

I added examples for save_iheatmap and setup_colorbar_grid

I added statement of need into README, similar to what was in vignette and paper.

Added to README

Edited vignette to make more clear [basic idea is that add_*_clustering functions do the work of clustering for you, but if you want to do clustering yourself there are are lower level functions for adding dendrogram or cluster annotation]

Have changed description field to avoid this

The main comments I haven't resolved:

I haven't been able to reproduce these warnings, so not sure how best to go about addressing them!

README & vignette don't make performance claims, the goal is not better performance than other similar packages, but more features & flexibility. Should I add performance tests?

maelle commented 7 years ago

Thanks @AliciaSchep! Nice Work!

Regarding what you say about performance do you cite these other packages in a Vignette? That'd be enough. 😊

@andeek and @carlganz are you happy with the changes? Please tell me if you don't have time to have a look, in which case I'll do that.

AliciaSchep commented 7 years ago

Thanks @maelle. With regards to other related packages, I cite them in README and paper.md, but not currently in vignette -- I can add that there as well.

maelle commented 7 years ago

@AliciaSchep yep maybe that'd be better because when you install the package from CRAN you often don't see the README&paper. In any case IMO just citing them is enough as regards performance. :-)

AliciaSchep commented 7 years ago

Great point about CRAN, hadn't thought of that -- I added mention of related packages to vignette as well.

maelle commented 7 years ago

@andeek @carlganz will you have a chance to look at the changes made by @AliciaSchep? :-)

carlganz commented 7 years ago

I will take a look tomorrow.

andeek commented 7 years ago

I would like to, but I'm turning in my dissertation to my committee next week. I'm sorry to drop the ball, I understand if you can't wait for that.

AliciaSchep commented 7 years ago

@andeek just turned my dissertation over to my committee last week, so totally understand... I think delay is fine, although will defer to what @maelle thinks is best. Good luck wrapping up the dissertation!

On a related note, @carlganz and @maelle, the package is currently broken as a result of update in plotly... fix in progress https://github.com/AliciaSchep/iheatmapr/pull/7

maelle commented 7 years ago

Wow congratulations all around for the dissertations @andeek @AliciaSchep! :rocket: :tada:

If Alicia is ok with waiting and if Andee is motivated to have a look at the package again after turning in her dissertation and a break I guess, it's fine. But Andee you can also choose to drop the ball, as you wish.

:four_leaf_clover: for the fix Alicia!

maelle commented 7 years ago

@AliciaSchep were you able to fix the package?

AliciaSchep commented 7 years ago

@maelle - yes! I ended up making quite a few changes so as to remove the plotly R package dependency so iheatmapr can have more control over version of plotly.js and how to pass data to plotly.js, and less reliance on internal functioning of plotly R package, which can change. This has additional benefit of allowing for iheatmapr specific shiny handlers -- I added a section to vignette about shiny integration.

maelle commented 7 years ago

@AliciaSchep awesome, great work! :ok_hand:

@carlganz if you can have a look at the package for approving the answer to your review, that'd be great. 😉

carlganz commented 7 years ago

I will work on this today.

carlganz commented 7 years ago

I only had a few small issues, and they have all been addressed:

I also took a look at the additional javascript code associated with the HTMLwidget, and it is very well done.

I've checked everything off in my review above. Let me know if you need any additional feedback.

maelle commented 7 years ago

@carlganz thanks a lot! :rocket: And thanks a bunch for even looking at the new code.

@AliciaSchep are you ok with waiting for the second reviewer to have a look again? Otherwise I can have a look at it myself next week before approving it.

andeek commented 7 years ago

@AliciaSchep @maelle I'll have time to look this weekend if you can hold off until Monday? Thanks for the patience!

maelle commented 7 years ago

@andeek oh I thought you'de be unavailable for a longer time, sorry! Thank you so much! And congrats on the thesis I suppose. :wink:

andeek commented 7 years ago

Hi all,

I've had a second pass and everything looks great. All of my questions were addressed. I also had a short look at the JS code and it looks very nice.

I do have one question: By changing to using the JS library (instead of the R package), should you reference the JS library instead of the R package in the paper?

Other than that, everything looks very nice. Easy to install now, and all tests pass on my machine. Great job! Let me know if I need to do anything more.

AliciaSchep commented 7 years ago

Thanks @carlganz and @andeek!

Re the new JS code, the main reason it's nice is that it was derived from a previous version of the plotly R package JS code -- I put a note at the top // Function adapted from Plotly R Package 3.60, but perhaps I should give additional credit elsewhere? I was thinking of adding an acknowledgements section to README and/or vignette, could mention it there or more formally in package metadata.

Also, since I am now including the JS code from plotly should I adjust the package level author, copyright, and license info to reflect that? The plotly LICENSE is included in the inst/htmlwidgets/lib/plotlyjs folder. For another package that I submitted to Bioconductor that included a C++ library from a third party, I included an AUTHORS file and COPYRIGHTS file to very clear indicate that the C++ library was the work of other authors. Would a similar strategy be appropriate here as well?

Re updating the paper to reference JS library, I agree that it would be appropriate to cite the JS library but think that it should still also cite the R package. I'll update to cite both.

maelle commented 7 years ago

Many thanks @andeek! 👌

@AliciaSchep I'll do some last checks next week 😺

maelle commented 7 years ago

@AliciaSchep reg your questions I don't know, we can ask in the slack/on the forum 😉

But as a quick answer now here you can see an example of an author list where they acknowledge the authors of code that was adapted from elsewhere.

maelle commented 7 years ago

@AliciaSchep Looking at the package right now, looks great. Could you add the rOpenSci footer to the bottom of the README? This is the code [![ropensci_footer](http://ropensci.org/public_images/github_footer.png)](http://ropensci.org) .

Reg. your question about the new JS code, was it settled or should we try finding an answer via the Slack or forum? I think that's the last thing before accepting your package.

After acceptance I'll ask you to transfer the repo to rOpenSci, and you'll need to:

But first let's solve this JS code issue. :wink:

AliciaSchep commented 7 years ago

Thanks @maelle. I have not yet figured out the JS code, been meaning to post a question, but haven't gotten around to it yet. Any suggestion of best place to post question? Not very familiar with the Slack, so don't know what 'channel' is appropriate

maelle commented 7 years ago

general is fine!

maelle commented 7 years ago

@AliciaSchep did the answer you got on slack help you?

AliciaSchep commented 7 years ago

@maelle yes it did, sorry i haven't made the change yet, will try to do so this weekend

AliciaSchep commented 7 years ago

@maelle, I finally updated the DESCRIPTION to add the plotly.js authors, using comment to indicate that they are authors of the js package, as suggested by Jim on ropensci slack and similar to what was done in package you suggested. I also added an acknowledgment section, in which I included a thanks to plotly r package for code that was adapted for this package. Let me know if you have additional suggestions for making sure appropriate credit is being given! I also added the ropensci footer

maelle commented 7 years ago

Great, now all problems have been solved as far as I can tell, good work! Approved!

To-dos:

Welcome aboard! We'd also love a blog post about your package, either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development. ((https://ropensci.org/blog/). Let me know if you are interested.

AliciaSchep commented 7 years ago

woohoo! I transferred the repo to ropensci and added the peer review badge. @maelle For CI, what are the new links? Anything else I need to do setup the CI? I noticed that OpenCPU CI sent me an email saying build had failed, due to BiocInstaller not being installed (package has one BioC dependency, S4Vectors).

AliciaSchep commented 7 years ago

@carlganz and @andeek may I add you as "rev" to the package description? thanks again for reviewing the package! (feel free to let me know either here or ropensci slack or email-- aschep is my gmail address)

maelle commented 7 years ago

@AliciaSchep awesome, thanks for transferring it!

jeroen commented 7 years ago

Don't worry about OpenCPU, it doesn't really support BioConductor yet :)

maelle commented 7 years ago

Thanks @jeroen

@AliciaSchep I forgot to mention to update the links in DESCRIPTION

AliciaSchep commented 7 years ago

I updated the links in Description & README. I tried doing the zenodo thing, but the repository doesn't show up in the list of repos to be able to archive? Is there some kind of access that has to be set @maelle?