-
-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
iheatmapr #107
Comments
Editor checks:
Editor commentsThanks for your submission @AliciaSchep! 😀 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.
|
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. |
Great but note that 100% coverage isn't compulsory 😉 |
@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. |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 3 Review CommentsThis package provides R users with a collection of easy-to-use functions for iteratively building complex interactive heatmaps. The 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 A few suggestions:
Hope that helps. Kind Regards, |
Thanks a lot for your review @carlganz ! |
@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. |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Paper (for packages co-submitting to JOSS)The package contains a
Functionality
Estimated hours spent reviewing: 3 Review CommentsOverall, 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 improvementPerhaps you could explain the difference between Documentation
Paper (for packages co-submitting to JOSS)
Functionality
|
Thanks @andeek for the review! I hope to have time soon to address all issues in both reviews. |
Thanks a lot @andeek ! 😸 @AliciaSchep now the two reviews are in, let us know if you have any question! |
@AliciaSchep I see you've done some work on the package, do you think you'll soon be able to answer to the reviews? 😉 |
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... |
No problem! I work on Windows, so if you need me to test anything, just ask 😉 |
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) |
Oh cool, nice catch! 🎣 |
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
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? |
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. |
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. |
Many thanks @andeek! 👌 @AliciaSchep I'll do some last checks next week 😺 |
@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. |
@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 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. 😉 |
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 |
general is fine! |
@AliciaSchep did the answer you got on slack help you? |
@maelle yes it did, sorry i haven't made the change yet, will try to do so this weekend |
@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 |
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. |
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 awesome, thanks for transferring it!
|
Don't worry about OpenCPU, it doesn't really support BioConductor yet :) |
Thanks @jeroen @AliciaSchep I forgot to mention to update the links in DESCRIPTION |
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? |
@AliciaSchep I’ve just made you admin of the repo so now it should work, sorry about that! |
Thanks @maelle, it shows up now! Question about timing of things -- I want to now submit package to CRAN, should I do that before doing the DOI & JOSS submission? Does it matter? |
Hi @AliciaSchep, as far as I know it doesn't matter but if you wait before the CRAN submission then you can have the JOSS paper in citation so that users of the CRAN version know how to cite your package. 😊 |
@AliciaSchep Are you going to submit the package to JOSS soon? For info yesterday I submitted a package to CRAN after JOSS acceptance, this is how CITATION looks like. https://github.com/ropensci/rtimicropem/blob/master/inst/CITATION If you do the optional item of asking reviewers whether they'd like to be included in DESCRIPTION as "rev", when submitting the package to CRAN you'll have to build it on R-devel (or wait until R3.5 :-)) because otherwise this non-standard role isn't translated well from Authors@R to Authors. |
@AliciaSchep any plan to submit the package to JOSS soon? |
I have added the reviewers to the DESCRIPTION and made a release. For JOSS submission, there is a dropdown of suggested editors -- any recommendation there? |
I actually don't think it makes a difference but when I submitted a paper to JOSS I chose Karthik because he's an rOpenSci editor as well 😉 |
Did you ask/inform the reviewers that they were added to DESCRIPTION? I've never seen anyone say no but better to ask. :-) |
Yes I asked the reviewers, both were fine with it! |
And now have also submitted to JOSS! Thanks @maelle |
Awesome, closing this issue! |
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.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
paper.md
with a high-level description in the package root or ininst/
.Detail
Does
R CMD check
(ordevtools::check()
) succeed? Paste and describe any errors or warnings:Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:
If this is a resubmission following rejection, please explain the change in circumstances:
If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:
The text was updated successfully, but these errors were encountered: