Submission: colocr #243
Comments
|
Thanks @MahShaaban! 😀 Before I proceed with regular checks since the package has a Shiny app could you have a look at our brand new specific guidance for Shiny apps especially their testing? https://ropensci.github.io/dev_guide/building.html#interactivegraphical-interfaces https://ropensci.github.io/dev_guide/building.html#testing You can ask any question here / on discuss.ropensci.org / Slack (had you gotten an invite to our Slack yet?) |
|
Hey, @maelle I had a look at links.
I don't think I've received any invites on slack. |
Ah sorry! But I don't see where in the tests? In particular, "shinytest" isn't written in DESCRIPTION like "testthat" is?
I've now sent you an invite to the Slack. |
|
Thanks for the slack invite, @maelle Regarding the shiny tests. The tests are in the app directory, |
|
To be honest I am very new to the testing ofShiny apps inside packages, and once your package is onboarded it'll probably be an example linked from the gitbook. 😸 Why test the app separately? What does it mean exactly? Your Travis config file looks normal to me 🤔 |
|
I am also new to I wanted to test the app separately cuz I intended to make it stand-alone and I have a prototype of it on shinyapps.io, here. I have the app as a separate repo on github, here. This repo is added to the package repo as a submodule. The app repo contains separate |
|
Ah interesting! Could you ask in the forum/Slack what best practice is? One thing that I still wonder about is whether your package's README should include a badge of the app's build status. I'll make other checks as soon as I can. |
|
Alright. Will do. Thanks |
|
Do you want to look into code generation before your package undergoes review? I tagged you in a commit, see links to apps generating code here. |
|
✖ use '<-' for assignment instead of '='. '<-' is
the standard, and R users and developers are used it and it
is easier to read your code for them if you use '<-'.
R\methods.R:367:12
R\methods.R:368:12
R\methods.R:369:5
✖ 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
inst\extdata\manual_analysis.R:6:1
R\methods.R:378:1
R\methods.R:379:1
R\methods.R:380:1
R\methods.R:381:1
and WORD FOUND IN
caculated coloc_test.Rd:23
determin roi_select.Rd:42
differen roi_show.Rd:19
fluoresence description:1
indecies intensity_get.Rd:15
indicies roi_show.Rd:16
manders coloc_test.Rd:12
pearsons coloc_test.Rd:12
reslution roi_show.Rd:24
Thes roi_select.Rd:36 |
|
I made the following changes
|
|
Cool, I'll now look for reviewers. Could you please add this badge to the README of your package: |
Editor checks:
Editor commentsAlready tackled 😉 Reviewers: @haozhu233 @seaaan |
|
Thanks @haozhu233 and @seaaan for accepting to review this package! Your reviews are due on 2018-09-03. Here are links to our reviewer guide and review template (as you're both experienced rOpenSci reviewers/authors comments on the guide are welcome in e.g. Slack but don't feel obliged to review both the package and the reviewing guide 😉). |
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: Review CommentsI tested on two computers: Windows 10, running R 3.5.1 and Ubuntu 16.04 running R 3.4.4.
The method of colocalization analysis is not new; the purpose of Below I have written comments based on the checkbox sections listed above. An overview of the most significant issues that I discovered:
More minor issues are named below. (Note that I have pointed out typos using this format: "the typo" -> "the correction".) The package is pretty nicely done and most of my comments are relatively minor. Good job! DocumentationStatement of needGood overview! "at loss" -> "at a loss" (Note that those two corrections should also be made in the vignette.) InstallationI don't understand what the "no submodules!" and "@development" mean in the installation guidelines. Can you explain (on the README) what that means and why? It took me a while to get the package installed on Ubuntu. Specifically, VignetteUnsuccessful building of vignetteThe vignette did not build successfully on either the Windows or the Ubuntu computer. This also prevented I figured out (via rstudio/rmarkdown#228) that changing the header of the vignette to include the following lines would fix it: This fixed the issue on both the Windows and Ubuntu computers. The vignette installed fine when installing with Vignette accessibilityI suggest adding a link to an HTML version of the vignette on your github README. As it is now, I have to install the package before I can read the vignette, which is a barrier to me installing it because I can't see very well what it does. Either knit the vignette to a github markdown document and link to that or, once it's on CRAN, link to the web version of the vignette that CRAN hosts. Vignette contentappropriatness -> appropriateness acheived -> achieved The reset of the anlysis -> the result (?) of the analysis Pearson’s Coefficeint Correlation -> Pearson correlation coefficient co-distrubuted -> co-distributed straight forward -> straightforward three steps only -> only three steps functionionality -> functionality encampasing -> encompassing resuliontion -> resolution Arguable -> arguably seleciton -> selection sampe -> sample Although -> this sentence should be combined with the next one, otherwise "Although" doesn't make sense rational -> rationale separatly -> separately through and formal -> thorough and formal correltion -> correlation Note that the vignette installation instructions differ from the README instructions. Maybe hide the startup messages that appear after In the "Getting started" section, give some background on what the image is, what the "appropriate parameters" are and how you determine them and evaluate their appropriateness. What are the cells in the image and what are they stained for? What is the goal of the ROIs? At the example setting, the five cells in the upper middle all appear enclosed in one region together, is that the goal? I would have thought to make a separate regions for each cell. Why do you use Some of the background I am asking for in the "Getting started" section is provided in the "Detailed Example" section. I might just skip the "Getting started" section altogether and add a little more background to the "Detailed Example" section. Why is the "Manders Overlap Coefficient" abbreviated with "SCC"? And why does the link go to a page about the Spearman's rank correlation coefficient? Is the link to the shiny app running on shinyapps.io the same as the app that comes with the package? "calling four different functions in order" but there are five listed What are you looking for in the Does the labeling of individual regions of interest just label the n largest contiguous regions? This is not totally clear from the vignette. In the colocalization benchmark source, it would be helpful to know what results are expected from the particular images chosen. Does Function documentation
Examples
Community guidelinesThere are no guidelines for how to contribute to the package. FUNCTIONALITYInstallationInstallation from github mostly works fine. I had some issues as described in the documentation described above. FunctionalityThe functionality described in the vignette and documentation is present and works just fine for the most part!
One issue I thought about is what if you want to correlate the intensity of more than two channels? Another issue I thought about is whether there could be a higher-level function to apply thresholds and get co-localization statistics for a bunch of images at once. I imagine relatively few cases where the user would want to repeat the code over and over for every image. Maybe a function that takes a vector of images and vectors for each argument to I don't really understand how Maybe import and re-export the functions from It would be nice to have more descriptive names for the results of Suggested APICurrently, the basic workflow is to do: It would be nice if you could do something like this: The benefits I see are that it's pipeable and that you don't have to separately keep track of This is not a requirement by any means, just something that I think would be nice! Performance
The app runs kind of slowly for me. It's not unusable by any means, but if there's some way you could speed it up (eg subsample the image to set the threshold and other parameters initially and then have a button to push to do the full analysis), that would be nice. Automated checks
|
|
Thanks for your thorough review @seaaan! 😀 |
|
@haozhu233 just a reminder that your review is due on 2018-09-03. Thank you! 😺 |
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:
|
|
Thanks a lot for your review @haozhu233! 😀 @MahShaaban now both reviews are in. |
|
I wrote two tests to check whether the output of the app is reproduced using the same input parameters. Onc test used the The tests are in a file called PS. @seaaan noticed before that the part of the vignette where I check the reproduction of the app output from the same input returns |
|
Interesting. Can you add a more minimal example using imager and data that's not in colocr so that I might run it and we can post in imager repo? |
|
I am not sure how to make a minimal example in this case. So far, I've been checking the final outputs of either
My current thought is to build |
|
@MahShaaban I don't see the test script with |
|
Sorry, I forgot to link to the test script in the |
|
I traced the difference between the calculated correlations on ubuntu and windows to very first step in loading the images. I think this (dahtah/imager#41) is related, although neither the maintainer nor the user followed up on the issue yet! I found that the pixel values of the images in the On ubuntu On windows Notice the difference starting at the 4th decimal point. I am showing the I couldn't go beyond that as |
|
I used |
|
Cool! In that case why not switch the whole package to magick? 😉 |
|
I would second maelle's suggestion to try and switch to magick. It is a much more comprehensive and reliable image toolkit. Is there particular functionality in imager that you are missing from magick? |
|
Thanks, @maelle @jeroen for the suggestion. I certainly don't mind looking into that. Although the package currently relies heavily on Here is the relevant part from the |
|
Thanks, see also this issue: ropensci/magick#136 In the latest dev version of magick you can find the morphology methods with > morphology_types()
[1] "Undefined" "Correlate" "Convolve" "Dilate"
[5] "Erode" "Close" "Open" "DilateIntensity"
[9] "ErodeIntensity" "CloseIntensity" "OpenIntensity" "DilateI"
[13] "ErodeI" "CloseI" "OpenI" "Smooth"
[17] "EdgeOut" "EdgeIn" "Edge" "TopHat"
[21] "BottomHat" "Hmt" "HitNMiss" "HitAndMiss"
[25] "Thinning" "Thicken" "Distance" "IterativeDistance"
[29] "Voronoi" I think the main features you use are:
I'm not sure what exactly For thresholding you can try |
|
Sounds great. I will be looking into that. Thanks @jeroen |
|
Approved! Thanks @MahShaaban for submitting and @seaaan @haozhu233 for your reviews! 😸 To-dos:
Should you want to awknowledge your reviewers in your package DESCRIPTION, you can do so by making them 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/). If you are interested, @stefaniebutland will be in touch about content and timing. We've started putting together a gitbook with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here. |
|
Thank you, everyone. I'd like to acknowledge your contributions @maelle, @haozhu233, @seaaan, and @jeroen if you don't mind. |
|
Awesome! Don't acknowledge my contributions, as mentioned here "Please do not list editors as contributors. Your participation in and contribution to rOpenSci is thanks enough!" 😉 (we mean it!) |
|
Hello @MahShaaban. Are you interested in writing a post about your package for the rOpenSci blog, 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/)? This link will give you many examples of blog posts by authors of onboarded packages so you can get an idea of the style and length you prefer: https://ropensci.org/tags/onboarding/. Here are some technical and editorial guidelines for contributing a post: https://github.com/ropensci/roweb2#contributing-a-blog-post. Please let me know what you think. |
|
Thanks, @stefaniebutland for this opportunity. I'd like to write a blog post about |
|
@MahShaaban What do you think about setting a deadline to submit a draft post? I'm happy to answer any questions you might have. |
|
Hey @stefaniebutland. I certainly don't mind that. I read the guides you referred to earlier and I think I will go with a short post intro. The idea is to adapt parts of the vignette that explains the goal of the package and how it with examples. If this is okay, I will start right away. |
|
If you're referring to a tech note (https://ropensci.org/technotes/), they don't require scheduling on a certain day of the week so please submit your draft when ready and I'll review it soon after.
Sounds good. Make sure it's different enough from the vignette. Good if you can lay out one cool example of what you can do with the package, rather than giving several examples. |
|
Okay. Thanks @stefaniebutland. |
|
@MahShaaban, @maelle just reminded me that this is your second package onboarding! I'm quite curious about package authors' motivations to submit multiple packages e.g. are there diminishing returns on author's effort on subsequent submissions? I know you indicated you prefer to write a tech note about Zero obligation to do more than you suggested! 😄 |
|
The truth is, I intended to write a blog post about this recent submission. The same happened the first time I submitted a package to ropensci. The reason I shied away from it is that I don't see how a detailed description of the package and the features could be different from the vignette! I am definitely willing to be educated on this. There might be different ways of writing or different aspects of the package that I should focus on while writing a blog post vs a package vignette. |
|
Sorry @MahShaaban I think I misunderstood when I thought you wanted to write a tech note. Yes your idea for a blog post: " to adapt parts of the vignette that explains the goal of the package and how it with examples" sounds good.
I think the blog post differs from the vignette in that the post should tell a bit of a story. Unlike a vignette, it's an opportunity to give your personal perspective on the package, like something you learned, or some really strong motivation for creating it. Was it your first Shiny app? Do you have any general tips for packages with Shiny apps? This might make the post interesting for people outside your field. (Thanks to @maelle for suggesting this to me when I asked her for advice.) Do you know of other users of your package? And how do they use it? Any of those things could go in the post. One of the big benefits of contributing a blog post is that it can get more eyes on your work. Once published, we tweet from rOpenSci to >20,000 followers and it gets picked up by R-weekly live and R-bloggers. With that, would you like to set a deadline for submitting a draft via pull request? Technical and editorial guidelines: https://github.com/ropensci/roweb2#contributing-a-blog-post. |
|
Hey @stefaniebutland, I just submitted a PR with a first draft of the blog post, ropensci-archive/roweb2#329. Please, let me know what you think. |
Summary
What does this package do? (explain in 50 words or less):
Conduct Co-localization Analysis of Fluorescence Microscopy Images
Paste the full DESCRIPTION file inside a code block below:
URL for the package (the development repository, not a stylized html page):
https://github.com/MahShaaban/colocr
Please indicate which category or categories from our package fit policies this package falls under *and why(? (e.g., data retrieval, reproducibility. If you are unsure, we suggest you make a pre-submission inquiry.):
[e.g., "data extraction, because the package parses a scientific data file format"]
data extraction
Who is the target audience and what are scientific applications of this package?
Biologists (no advanced R required)
Are there other R packages that accomplish the same thing? If so, how does
yours differ or meet our criteria for best-in-category?
The EBImage and the imager packages contain the algorithms for dealing with images. This package uses imager but is focused only on the functionality needed for conducting co-localization of multichannel images. In addition, the package provide a shiny app to inteactively perform the same analysis.
If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.
A pre-submission, Pre-submisstion Inquiry: colocr #241
Requirements
Confirm each of the following by checking the box. This package:
Publication options
paper.mdmatching JOSS's requirements 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: