search

Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

VennDetail #999

Closed guokai8 closed 5 years ago

guokai8 commented 5 years ago

Update the following URL to point to the GitHub repository of the package you wish to submit to Bioconductor

Confirm the following by editing each check box to '[x]'

I am familiar with the essential aspects of Bioconductor software management, including:

For help with submitting your package, please subscribe and post questions to the bioc-devel mailing list.

bioc-issue-bot commented 5 years ago

Hi @guokai8

Thanks for submitting your package. We are taking a quick look at it and you will hear back from us soon.

The DESCRIPTION file for this package is:

Package: VennDetail
Type: Package
Title: VennDetail Package Used for Extract Information from Venndiagram Plot
Version: 0.99.0
Author: Kai Guo, Brett McGregor
Maintainer: Kai Guo <guokai8@gmail.com>
Description: This package used for extract information from Venndiagram plot
License: GPL-2
Encoding: UTF-8
LazyData: True
Imports:
    utils,
    dplyr,
    purrr,
    tibble,
    magrittr,
    ggplot2,
    UpSetR,
    VennDiagram,
    grid
Depends:
LinkingTo:
Suggests:
    knitr,
    rmarkdown,
    testthat
VignetteBuilder: knitr
URL: https://github.com/guokai8/VennDetail
RoxygenNote: 6.1.1
biocViews:DataRepresentation,GraphAndNetwork
bioc-issue-bot commented 5 years ago

A reviewer has been assigned to your package. Learn what to expect during the review process.

IMPORTANT: Please read the instructions for setting up a push hook on your repository, or further changes to your repository will NOT trigger a new build.

mtmorgan commented 5 years ago

This package looks like it is useful to R users in general; I recommend that you submit it to CRAN instead.

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

guokai8 commented 5 years ago

Dear Reviewer, Thanks for your suggestion. Can I still try to submit my package to biocondutor? I am a bioinformatics PhD. I more prefer biocondcutor. Kai

guokai8 commented 5 years ago

Already remove the VennDetail.Rproj file

mtmorgan commented 5 years ago

Remember to increment the version number (0.99.1, 0.99.2, 0.99.3...) whenever you want to have your package checked again.

Yes you can still submit this to Bioconductor.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

88bca73 Update DESCRIPTION

guokai8 commented 5 years ago

Update the test folder. And version changed to 0.99.1. Fixed the test error.

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

22e8c4d Fixed the test error and update version

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

f87c5e8 update get function and version

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

4d21318 Update DESCRIPTION

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

a113728 Update DESCRIPTION

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR, WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

3e225cd add show generic function

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

bc0169e Update DESCRIPTION

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

27fb93a Update DESCRIPTION

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

guokai8 commented 5 years ago

This is my first package submission to Bioconductor. I apologize in advance for my lack of experience.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

496598c Add files via upload

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

17f887b Update DESCRIPTION

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

ea703af Update DESCRIPTION

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

577edd2 Update DESCRIPTION

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

guokai8 commented 5 years ago

Hi, I just add new function in VennDetail. Now it supports to extract wide format to see the unique or shared results. Kai

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

ce8553d Update DESCRIPTION

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

hpages commented 5 years ago

Hi @guokai8 ,

Here is some initial feedback for the VennDetail package. Overall the package needs more work.

Code quality

  1. show() is an S4 generic defined in the methods package. Don't redefine this generic. More generally speaking, you should never override an existing generic function. Instead, all you need to do is define a show() method for your venn objects. Note that even though you are currently defining such method, the method gets attached to your show() generic instead of to the original show() generic defined in the methods package. This breaks many things downstream, starting with your show() method not being called when entering the name of a venn object at the command line (followed by \<ENTER>):

    library(VennDetail)
set.seed(123)
A <- sample(1:5, 2, replace=FALSE)
B <- sample(1:5, 4, replace=FALSE)
res <- venndetail(list(A=A, B=B))
res  # this doesn't call show(res) as it should

    BTW, calling show(res) explicitly shows some strange output for this object:

    show(res)
# === Here is the detail of Venndiagram===
# Total results:  5 x 2 
# Total sets is: 3 
#    Group Detail
# 1 Shared      4
# 2      A      2
# 3      B      3
# 4      B      5
# 5      B      1
# ... with -1 more rows

    See last line: "with -1 more rows" !?

  2. Similar situation with merge(). This function is already defined (as an S3 generic) in the base package. Do NOT override it with your own S4 generic. Again this will break many things when people use your package in conjunction with other Bioconductor or CRAN package. You are only allowed to define methods for it, not to override the existing generic function. Note that even though base::merge() is an S3 method, you can define an S4 or method for it.

  3. Why mix S3 generics (e.g. result(), detail()) with S4 generics (e.g. dplot(), rowjoin(), Get(), etc...) when introducing new generic functions? Bioconductor recommends that packages implement S4 classes instead of S3 classes. Also new generics introduced to manipulate S4 objects should preferably be defined as S4 generics instead of S3 generics.

  4. Don't put curly brackets ({ .. }) around the call to standardGeneric() in your setGeneric() statements.

  5. Plotting a venn object with 6 or more sets doesn't seem to work:

    library(VennDetail)
A <- sample(1:1000, 400, replace=FALSE)
B <- sample(1:1000, 600, replace=FALSE)
C <- sample(1:1000, 350, replace=FALSE)
D <- sample(1:1000, 550, replace=FALSE)
E <- sample(1:1000, 450, replace=FALSE)
F <- sample(1:1000, 750, replace=FALSE)
res <- venndetail(list(A=A, B=B, C=C, D=D, E=E, F=F), plot=TRUE)  # no plot produced
plot(res)  # no plot produced

  6. All the logic for plotting a venn object is implemented in S3 method plot.venn(). Why repeat that logic at the end of the venndetail() constructor when you could just call plot() on the newly created venn object? Avoiding code duplication is part of the DRY (Don't Repeat Yourself) principle, an important principle in good software engineering practices.

Vignette

  1. Always put a space after commas (,) and periods (.).

  2. "offering" instead of "offerings" in "no R package offerings functions to extract"

  3. The dependencies listed in the vignette don't match those listed in the DESCRIPTION file. It's actually not useful to list them in the vignette since they get automatically installed when installing the package.

  4. Never install a Bioconductor package from GitHub. The proper way to install a Bioconductor package is explained here: https://bioconductor.org/install/ Either provide that link, or show the following code in your vignette:

    if (!requireNamespace("BiocManager"))
    install.packages("BiocManager")
BiocManager::install("VennDetail")

    Or both. Note that the above code should not get evaluated when the vignette gets built (e.g. with R CMD build) which means that eval=FALSE should be used in the opening line of the code chunk.

  5. Please hide the output of library(VennDetail) by using results="hide" in the opening line of the corresponding code chunk.

  6. No need to call library(VennDetail) twice.

  7. What about the empty "Support shiny now" section?

  8. Overall the vignette is too minimalist. It should show how to use VennDetail on real data.

Coding style

  1. S4 class names capitalized i.e. Venn instead of venn.

  2. Put spaces around <-.

  3. Put a space after commas (,).

  4. Please remove the trailing semicolons in your code. They are not needed and hurt readability. This applies to the code chunks in the vignette and to the examples in the man pages.

Please address at your earliest convenience. Thanks!

Regards, H.

guokai8 commented 5 years ago

Hi, I will revised ASAP. Thanks! Kai

On 21 Mar 2019, at 11:50 AM, hpages notifications@github.com wrote:

Hi @guokai8 https://github.com/guokai8 ,

Here is some initial feedback for the VennDetail package. Overall the package needs more work.

Code quality

show() is an S4 generic defined in the methods package. Don't redefine this generic. More generally speaking, you should never override an existing generic function. Instead, all you need to do is define a show() method for your venn objects. Note that even though you are currently defining such method, the method gets attached to your show() generic instead of to the original show() generic defined in the methods package. This breaks many things downstream, starting with your show() method not being called when entering the name of a venn object at the command line (followed by ):

library(VennDetail) set.seed(123) A <- sample(1:5, 2, replace=FALSE) B <- sample(1:5, 4, replace=FALSE) res <- venndetail(list(A=A, B=B)) res # this doesn't call show(res) as it should BTW, calling show(res) explicitly shows some strange output for this object:

show(res)

=== Here is the detail of Venndiagram===

Total results: 5 x 2

Total sets is: 3

Group Detail

1 Shared 4

2 A 2

3 B 3

4 B 5

5 B 1

... with -1 more rows

See last line: "with -1 more rows" !?

Similar situation with merge(). This function is already defined (as an S3 generic) in the base package. Do NOT override it with your own S4 generic. Again this will break many things when people use your package in conjunction with other Bioconductor or CRAN package. You are only allowed to define methods for it, not to override the existing generic function. Note that even though base::merge() is an S3 method, you can define an S4 or method for it.

Why mix S3 generics (e.g. result(), detail()) with S4 generics (e.g. dplot(), rowjoin(), Get(), etc...) when introducing new generic functions? Bioconductor recommends that packages implement S4 classes instead of S3 classes. Also new generics introduced to manipulate S4 objects should preferably be defined as S4 generics instead of S3 generics.

Don't put curly brackets ({ .. }) around the call to standardGeneric() in your setGeneric() statements.

Plotting a venn object with 6 or more sets doesn't seem to work:

library(VennDetail) A <- sample(1:1000, 400, replace=FALSE) B <- sample(1:1000, 600, replace=FALSE) C <- sample(1:1000, 350, replace=FALSE) D <- sample(1:1000, 550, replace=FALSE) E <- sample(1:1000, 450, replace=FALSE) F <- sample(1:1000, 750, replace=FALSE) res <- venndetail(list(A=A, B=B, C=C, D=D, E=E, F=F), plot=TRUE) # no plot produced plot(res) # no plot produced All the logic for plotting a venn object is implemented in S3 method plot.venn(). Why repeat that logic at the end of the venndetail() constructor when you could just call plot() on the newly created venn object? Avoiding code duplication is part of the DRY (Don't Repeat Yourself) principle, an important principle in good software engineering practices.

Vignette

Always put a space after commas (,) and periods (.).

"offering" instead of "offerings" in "no R package offerings functions to extract"

The dependencies listed in the vignette don't match those listed in the DESCRIPTION file. It's actually not useful to list them in the vignette since they get automatically installed when installing the package.

Never install a Bioconductor package from GitHub. The proper way to install a Bioconductor package is explained here: https://bioconductor.org/install/ https://bioconductor.org/install/ Either provide that link, or show the following code in your vignette:

if (!requireNamespace("BiocManager")) install.packages("BiocManager") BiocManager::install("VennDetail") Or both. Note that the above code should not get evaluated when the vignette gets built (e.g. with R CMD build) which means that eval=FALSE should be used in the opening line of the code chunk.

Please hide the output of library(VennDetail) by using results="hide" in the opening line of the corresponding code chunk.

No need to call library(VennDetail) twice.

What about the empty "Support shiny now" section?

Overall the vignette is too minimalist. It should show how to use VennDetail on real data.

Coding style

S4 class names capitalized i.e. Venn instead of venn.

Put spaces around <-.

Put a space after commas (,).

Please remove the trailing semicolons in your code. They are not needed and hurt readability. This applies to the code chunks in the vignette and to the examples in the man pages.

Please address at your earliest convenience. Thanks!

Regards, H.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Bioconductor/Contributions/issues/999#issuecomment-475312334, or mute the thread https://github.com/notifications/unsubscribe-auth/AIqsm70a4yd4gC2JPGTrdwHc5Em6E9xMks5vY7hegaJpZM4aoGMf.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

cd80f14 update to 0.99.12

guokai8 commented 5 years ago

Hi, Thanks for you kindly suggestions. Below is the point by point revised.

Code quality

  1. show() is an S4 generic defined in the methods package. Don't redefine this generic. More generally speaking, you should never override an existing generic function. Instead, all you need to do is define a show() method for your venn objects. Note that even though you are currently defining such method, the method gets attached to your show() generic instead of to the original show() generic defined in the methods package. This breaks many things downstream, starting with your show() method not being called when entering the name of a venn object at the command line (followed by ):

Response: show function was re-designed as a S3 function.

library(VennDetail)
set.seed(123)
A <- sample(1:5, 2, replace=FALSE)
B <- sample(1:5, 4, replace=FALSE)
res <- venndetail(list(A=A, B=B))
res  # this doesn't call show(res) as it should

Response: The problem have been fixed.

BTW, calling show(res) explicitly shows some strange output for this object:

Response: The problem have been fixed as show function was not directly export now

  1. Similar situation with merge(). This function is already defined (as an S3 generic) in the base package. Do NOT override it with your own S4 generic. Again this will break many things when people use your package in conjunction with other Bioconductor or CRAN package. You are only allowed to define methods for it, not to override the existing generic function. Note that even though base::merge() is an S3 method, you can define an S4 or method for it.

Response: merge function was re-designed as a S3 function.

  1. Why mix S3 generics (e.g. result(), detail()) with S4 generics (e.g. dplot(), rowjoin(), Get(), etc...) when introducing new generic functions? Bioconductor recommends that packages implement S4 classes instead of S3 classes. Also new generics introduced to manipulate S4 objects should preferably be defined as S4 generics instead of S3 generics.

Response: The result() ,detail() function were re-designed as S4 function

  1. Don't put curly brackets ({ .. }) around the call to standardGeneric() in your setGeneric() statements.

Response: The curly brackets have been removed in setGeneric() statements.

  1. Plotting a venn object with 6 or more sets doesn't seem to work:

Response: The issue have been fixed.

  1. All the logic for plotting a venn object is implemented in S3 method plot.venn(). Why repeat that logic at the end of the venndetail() constructor when you could just call plot() on the newly created venn object? Avoiding code duplication is part of the DRY (Don't Repeat Yourself) principle, an important principle in good software engineering practices.

Response: The plot parameter in the venndetail have been removed

Vignette

  1. Always put a space after commas (,) and periods (.).

Response: spaces were add after commas and periods.

  1. "offering" instead of "offerings" in "no R package offerings functions to extract"

__Response: "offerings" have been changed to "offering"

  1. The dependencies listed in the vignette don't match those listed in the DESCRIPTION file. It's actually not useful to list them in the vignette since they get automatically installed when installing the package. Response: We rewrite the vignette so the dependencies part was removed

10.Never install a Bioconductor package from GitHub. The proper way to install a Bioconductor package is explained here: https://bioconductor.org/install/ Either provide that link, or show the following code in your vignette:

if (!requireNamespace("BiocManager")) install.packages("BiocManager") BiocManager::install("VennDetail") Or both. Note that the above code should not get evaluated when the vignette gets built (e.g. with R CMD build) which means that eval=FALSE should be used in the opening line of the code chunk.

Response: We rewrite the vignette. This part have been added.

  1. Please hide the output of library(VennDetail) by using results="hide" in the opening line of the corresponding code chunk.

Response: The results="hide" have been added.

  1. No need to call library(VennDetail) twice.

Response: Now library(VennDetail) at the beginning.

  1. What about the empty "Support shiny now" section?

Response: This part have been extended.

  1. Overall the vignette is too minimalist. It should show how to use VennDetail on real data. Response:The vignette was rewrited.

Coding style

  1. S4 class names capitalized i.e. Venn instead of venn.

Response: "venn" was changed to "Venn".

  1. Put spaces around <-.

Response: spaces were added.

  1. Put a space after commas (,).

Response: spaces were added.

  1. Please remove the trailing semicolons in your code. They are not needed and hurt readability. This applies to the code chunks in the vignette and to the examples in the man pages.

Response: semicolons were removed.

Thanks again. Kai

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR, WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

3c2a2ed Update DESCRIPTION

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

guokai8 commented 5 years ago

Hi @hpages , I just wonder if you receive the revised information I upload. Best, Kai

hpages commented 5 years ago

Hi @guokai8 ,

Thanks for these improvements. Some of the items listed in my initial feedback still need your attention. Also there are still some important usability issues with the package. See below.

Regards, H.

Previous items that still need your attention

Vignette

Thanks for the improved vignette. Here are a few more things about the vignette:

  1. The "Software Implementation" section is more about software usage than software implementation so its title is misleading.

  2. The title of the "Support Shiny Now" subsection should probably be modified now that the shiny web app is available.

  3. This sentence needs a verb and a period:

    A shiny web application VennDetail for those who are not familiar
with the R analysis platform

Other issues

  1. It doesn't seem right that the number of total sets for res7 is less than for res6 in the following example:

    library(VennDetail)
set.seed(123)
A <- sample(1:1000, 400, replace=FALSE)
B <- sample(1:1000, 600, replace=FALSE)
C <- sample(1:1000, 350, replace=FALSE)
D <- sample(1:1000, 550, replace=FALSE)
E <- sample(1:1000, 450, replace=FALSE)
F <- sample(1:1000, 750, replace=FALSE)
G <- sample(1:1000, 425, replace=FALSE)
res6 <- venndetail(list(A=A, B=B, C=C, D=D, E=E, F=F))
res7 <- venndetail(list(A=A, B=B, C=C, D=D, E=E, F=F, G=G))
res6
# === Here is the detail of Venndiagram ===
# Total results:  970 x 2 
# Total sets is: 62 
#    Group Detail
# 1 Shared    217
# 2 Shared     72
# 3 Shared    456
# 4 Shared    291
# 5 Shared    424
# 6 Shared      5
# ... with 964 more rows ...
res7
# === Here is the detail of Venndiagram ===
# Total results:  113 x 2 
# Total sets is: 15 
#    Group Detail
# 1 Shared    217
# 2 Shared    291
# 3 Shared    424
# 4 Shared      5
# 5 Shared     20
# 6 Shared    712
# ... with 107 more rows ...

    Note that these Total sets numbers (62 and 15) are in disagreement with VennDiagram::get.venn.partitions():

    library(VennDiagram)
nrow(get.venn.partitions(list(A=A, B=B, C=C, D=D, E=E, F=F)))
[1] 63
nrow(get.venn.partitions(list(A=A, B=B, C=C, D=D, E=E, F=F, G=G)))
[1] 127

  2. venndetail() gives me an error when the number of input sets is > 8:

    H <- sample(1:1000, 750, replace=FALSE)
I <- sample(1:1000, 425, replace=FALSE)
res9 <- venndetail(list(A=A, B=B, C=C, D=D, E=E, F=F, G=G, H=H, I=I))
# Error in new("Venn", input = x, raw = raw, sep = sep, GroupNames = GroupNames,  : 
#   object 'res' not found

    If 8 is the maximum number of input sets supported by venndetail() then the function should fail gracefully with an informative error message. Also in addition to mentioning that "graphical visualization is usually limited to six or less gene sets" for the existing packages and web applications, the vignette could also mention what the limit is for VennDetail.

  3. Even though most of the time you use the term "groups" to design the input sets of the venndetail() function, sometimes you also use "set" for this. Note that you also use "sets" to design the Venn regions in the resulting diagram. This results in an overall documentation that lacks precision and clarity. Also this terminolgy is not following the usual convention which is to use "sets" for the input sets and "partitions" for the Venn regions. Please use this terminology instead of "groups" and "sets". Make sure to use it consitently everywhere (e.g. vignette, man pages, show method, etc...)

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

d54ec41 Update DESCRIPTION

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

hpages commented 5 years ago

Hi @guokai8 ,

I see that you've made changes to the package. Is the package ready for me to take another look at it?

Thanks, H.

guokai8 commented 5 years ago

Hi @hpages, I am still working on it. I will revise it ASAP. Thanks! Kai

guokai8 commented 5 years ago

Hi @hpages

Previous items that still need your attention

• About item 11: The undesired output of library(VennDetail) is still displayed. Looks like you might need to also use code chunk option message=FALSE in addition to results="hide".

Response: “message=FALSE” was added in the chunk

• About item 16: You still have a few places in your code with no spaces around the assignment operator e.g. in the README.md file etc... Also make sure to always use the left arrow (<-) for assignment (I see that you sometimes use =).

Response: all assignment operators with “= ” have been replaced with “<-”

• About item 17: You now have 2 spaces after the comma in many places e.g. in the examples for dplot etc... It should be 1 space only.

Response: extras spaces have been removed

• About item 18: Your code still has trailing semicolons in many places e.g. in the README.md file, in the examples for dplot etc... Make sure to remove them all.

Response: semicolons have been removed in all examples

Vignette

Thanks for the improved vignette. Here are a few more things about the vignette:

  1. The "Software Implementation" section is more about software usage than software implementation so its title is misleading.

    Response: “Software Implementation” was renamed as “Software Usage”

  2. The title of the "Support Shiny Now" subsection should probably be modified now that the shiny web app is available.

    Response: “Support Shiny Now” change to “Shiny web app”

  3. This sentence needs a verb and a period: 
    A shiny web application VennDetail for those who are not familiar   
with the R analysis platform

    Response: The sentence was changed to “Shiny web application”

    Other issues

  4. It doesn't seem right that the number of total sets for res7 is less than for res6 in the following example: 
    library(VennDetail)
set.seed(123)
A <- sample(1:1000, 400, replace=FALSE)
B <- sample(1:1000, 600, replace=FALSE)
C <- sample(1:1000, 350, replace=FALSE)
D <- sample(1:1000, 550, replace=FALSE)
E <- sample(1:1000, 450, replace=FALSE)
F <- sample(1:1000, 750, replace=FALSE)
G <- sample(1:1000, 425, replace=FALSE)
res6 <- venndetail(list(A=A, B=B, C=C, D=D, E=E, F=F))
res7 <- venndetail(list(A=A, B=B, C=C, D=D, E=E, F=F, G=G))
res6
# === Here is the detail of Venndiagram ===
# Total results:  970 x 2 
# Total sets is: 62 
#    Group Detail
# 1 Shared    217
# 2 Shared     72
# 3 Shared    456
# 4 Shared    291
# 5 Shared    424
# 6 Shared      5
# ... with 964 more rows ...
res7
# === Here is the detail of Venndiagram ===
# Total results:  113 x 2 
# Total sets is: 15 
#    Group Detail
# 1 Shared    217
# 2 Shared    291
# 3 Shared    424
# 4 Shared      5
# 5 Shared     20
# 6 Shared    712
# ... with 107 more rows ...

    Note that these Total sets numbers (62 and 15) are in disagreement with VennDiagram::get.venn.partitions():

    library(VennDiagram)
nrow(get.venn.partitions(list(A=A, B=B, C=C, D=D, E=E, F=F)))
[1] 63
nrow(get.venn.partitions(list(A=A, B=B, C=C, D=D, E=E, F=F, G=G)))
[1] 127

    Response: The issus was fixed.

    library(VennDetail)
set.seed(123)
A <- sample(1:1000, 400, replace=FALSE)
B <- sample(1:1000, 600, replace=FALSE)
C <- sample(1:1000, 350, replace=FALSE)
D <- sample(1:1000, 550, replace=FALSE)
E <- sample(1:1000, 450, replace=FALSE)
F <- sample(1:1000, 750, replace=FALSE)
G <- sample(1:1000, 425, replace=FALSE)
res6 <- venndetail(list(A=A, B=B, C=C, D=D, E=E, F=F))
res7 <- venndetail(list(A=A, B=B, C=C, D=D, E=E, F=F, G=G))
res6
=== Here is the detail of Venndiagram ===
Total results:  988 x 2 
Total sets is: 63 
Group Detail
1 Shared    453
2 Shared    993
3 Shared    961
4 Shared    758
5 Shared    626
6 Shared    361
... with 982 more rows ...
res7
=== Here is the detail of Venndiagram ===
Total results:  993 x 2 
Total sets is: 126 
Group Detail
1 Shared    993
2 Shared    758
3 Shared    626
4 Shared    614
5 Shared    540
6 Shared    411
... with 987 more rows ...

    The reason we got 126 compare with nrow(get.venn.partitions(list(A=A, B=B, C=C, D=D, E=E, F=F, G=G))) got 127 is that we remove the set with 0 element.

  5. venndetail() gives me an error when the number of input sets is > 8: 
    H <- sample(1:1000, 750, replace=FALSE)
I <- sample(1:1000, 425, replace=FALSE)
res9 <- venndetail(list(A=A, B=B, C=C, D=D, E=E, F=F, G=G, H=H, I=I))
# Error in new("Venn", input = x, raw = raw, sep = sep, GroupNames = GroupNames,  : 
#   object 'res' not found

    If 8 is the maximum number of input sets supported by venndetail() then the function should fail gracefully with an informative error message. Also in addition to mentioning that "graphical visualization is usually limited to six or less gene sets" for the existing packages and web applications, the vignette could also mention what the limit is for VennDetail.

    Response: We support unlimited groups after revised the code and the issue have been fixed. We also add explanation in the vignette.

    H <- sample(1:1000, 750, replace=FALSE)
I <- sample(1:1000, 425, replace=FALSE)
res9 <- venndetail(list(A=A, B=B, C=C, D=D, E=E, F=F, G=G, H=H, I=I))
res9
=== Here is the detail of Venndiagram ===
Total results:  1000 x 2 
Total sets is: 357 
        Group Detail
1          Shared    993
2          Shared    758
3          Shared    614
4          Shared    636
5          Shared    674
6 B_C_D_E_F_G_H_I    118
... with 994 more rows ...
  6. Even though most of the time you use the term "groups" to design the input sets of the venndetail() function, sometimes you also use "set" for this. Note that you also use "sets" to design the Venn regions in the resulting diagram. This results in an overall documentation that lacks precision and clarity. Also this terminology is not following the usual convention which is to use "sets" for the input sets and "partitions" for the Venn regions. Please use this terminology instead of "groups" and "sets". Make sure to use it consistently everywhere (e.g. vignette, man pages, show method, etc...)

    Response: We try to use “group” to define the input datasets and “set” or “subset” to define the shared or unique partitions since we think biologist are more familiar with these terminology. We revised the vignette and man pages to make it consistently.