waldronlab / MultiAssayExperiment

Bioconductor package for management of multi-assay data
https://waldronlab.io/MultiAssayExperiment/
69 stars 32 forks source link

lose some dependencies #199

Closed lawremi closed 6 years ago

lawremi commented 7 years ago

This package has a lot of dependencies for what I think is meant to be a core infrastructure package. For example, couldn't the shiny interface, venn diagrams, etc be in a separate package? Could also drop reshape2 and tidyr by just using stats::reshape().

LiNk-NY commented 7 years ago

Hi Michael, @lawremi Thanks for the suggestions. I will get to that soon. Regards, Marcel

lwaldron commented 7 years ago

A MultiAssayExperimentExtras package?

lawremi commented 7 years ago

There are really just two things, the shiny app and the venn diagram (upsetSamples). The venn diagram could just happen via Suggests and requireNamespace(). The shiny app just due to its complexity probably belongs as its own package, like MultiAssayExperimentShiny or something.

ttriche commented 7 years ago

R> library()$results[,"Package"] %>% regexPipes::grep("extra(s?)$", ignore=TRUE, value=TRUE)

[1] "bigmemoryExtras" [2] "DMRcateExtras" [3] "ggExtra" [4] "gridExtra" [5] "latticeExtra" [6] "optextras"

Seems reasonable...

--t

On Thu, Apr 27, 2017 at 9:30 AM, Levi Waldron notifications@github.com wrote:

A MultiAssayExperimentExtras package?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/waldronlab/MultiAssayExperiment/issues/199#issuecomment-297768469, or mute the thread https://github.com/notifications/unsubscribe-auth/AAARIpK4fk3D3rM5dKIdXeTe0mJOwaa5ks5r0MKXgaJpZM4NJiX2 .

ttriche commented 7 years ago

how does requireNamespace() work in this situation?

I will take my own question to bioc-devel, but it is an interesting point: the MAE data structure screams out for automated UpSet plotting, due to "holes" designed into the class. Yet, the dependencies for UpSetR are also nontrivial (as a bonus, these include gridExtra). So while it would be nice not to have those dependencies, the functionality is quite natural to keep track of a sprawling MAE. It seems like this approach of loadNamespace()/requireNamespace() and fail if not present is a somewhat sneaky way to avoid Depends: ?

http://r-pkgs.had.co.nz/namespace.html

Is this considered kosher? Something about it seems wrong.

--t

On Thu, Apr 27, 2017 at 9:43 AM, Michael Lawrence notifications@github.com wrote:

There are really just two things, the shiny app and the venn diagram ( upsetSamples). The venn diagram could just happen via Suggests and requireNamespace(). The shiny app just due to its complexity probably belongs as its own package, like MultiAssayExperimentShiny or something.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/waldronlab/MultiAssayExperiment/issues/199#issuecomment-297771853, or mute the thread https://github.com/notifications/unsubscribe-auth/AAARIgqkVmvQz1JrwVd9k53pi8-e8DCDks5r0MWkgaJpZM4NJiX2 .

LiNk-NY commented 7 years ago

From published packages:

> unname(grep("extra", c(BiocInstaller::all_group(), utils::available.packages(paste0(as.list(getOption("repos"))[["CRAN"]], "/src/contrib"))[, "Package"]), ignore.case = TRUE, value = TRUE))
 [1] "bigmemoryExtras"  "analogueExtra"    "DeducerExtras"    "dendroextras"     "extraBinomial"    "extracat"         "extraDistr"      
 [8] "extrafont"        "extrafontdb"      "extraTrees"       "factoextra"       "ggExtra"          "ggiraphExtra"     "gridExtra"       
[15] "kableExtra"       "latticeExtra"     "optextras"        "RedditExtractoR"  "RGtk2Extras"      "Rtextrankr"       "RYandexTranslate"
[22] "TraMineRextras"   "vcdExtra"         "ycinterextra"    

We actually use UpSetR::upset in the upsetSamples function so I don't think requireNamespace would work out unless we re-implement the upset algorithm in the package (unlikely).

We would have to move this feature into another package. The same would go with the shiny app.

Regards, Marcel

kasperdanielhansen commented 7 years ago

Making an Extra package seem obvious. As things mature, we might consider moving stuff from Extra into the base package, but I think for now we want to have it lean and mean, and then expect that most users will need the Extra for the time being.

On Thu, Apr 27, 2017 at 1:03 PM, Marcel Ramos notifications@github.com wrote:

From published packages:

unname(grep("extra", c(BiocInstaller::all_group(), utils::available.packages(paste0(as.list(getOption("repos"))[["CRAN"]], "/src/contrib"))[, "Package"]), ignore.case = TRUE, value = TRUE)) [1] "bigmemoryExtras" "analogueExtra" "DeducerExtras" "dendroextras" "extraBinomial" "extracat" "extraDistr" [8] "extrafont" "extrafontdb" "extraTrees" "factoextra" "ggExtra" "ggiraphExtra" "gridExtra" [15] "kableExtra" "latticeExtra" "optextras" "RedditExtractoR" "RGtk2Extras" "Rtextrankr" "RYandexTranslate" [22] "TraMineRextras" "vcdExtra" "ycinterextra"

We actually use UpSetR::upset in the upsetSamples function so I don't think requireNamespace would work out unless we re-implement the upset algorithm in the package (unlikely).

We would have to move this feature into another package. The same would go with the shiny app.

Regards, Marcel

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/waldronlab/MultiAssayExperiment/issues/199#issuecomment-297777153, or mute the thread https://github.com/notifications/unsubscribe-auth/AEuhn2MmbKcb6nNECJrzmRhQQDWvlYkoks5r0MpngaJpZM4NJiX2 .

lawremi commented 7 years ago

Sorry, but I don't get what you guys are talking about. All you need is

if (!requireNamespace("UpsetR")) {
  stop("Please install the UpsetR package to make venn diagrams")
}

Then move upsetR to Suggests. That's it.

lawremi commented 7 years ago

An "Extras" package is a meaningless way to group functionality. Please organize stuff into meaningful modules.

ttriche commented 7 years ago

This is a powerful feature, for users to be able to see how all of the samples fit together. e.g. For some of the TARGET sub-projects, there are different genotyping technologies, different focused (designed) subgroup experiments with assorted assay coverage, different numbers of samples (dx, rm, eoi, cr1, cr2), different assays... it gets really, really hard to track. Anything that can provide visual traction on the situation is super handy.

I am thrilled to discover that you have automated this! It took us weeks to straighten out which participants had what, plot it with UpSetR in a way that people could comprehend, and then we started receiving targeted validation from other cohorts... meanwhile it is quietly incorporated into the MAE package.

It may not be core to the data structures and data management that MAE is meant to perform, but the functionality will (I think) be used by a great many users if it is obvious how they should use it (i.e. put it in the vignette!)

thank you for adding this to the package.

--t

On Thu, Apr 27, 2017 at 10:03 AM, Marcel Ramos notifications@github.com wrote:

From published packages:

unname(grep("extra", c(BiocInstaller::all_group(), utils::available.packages(paste0(as.list(getOption("repos"))[["CRAN"]], "/src/contrib"))[, "Package"]), ignore.case = TRUE, value = TRUE)) [1] "bigmemoryExtras" "analogueExtra" "DeducerExtras" "dendroextras" "extraBinomial" "extracat" "extraDistr" [8] "extrafont" "extrafontdb" "extraTrees" "factoextra" "ggExtra" "ggiraphExtra" "gridExtra" [15] "kableExtra" "latticeExtra" "optextras" "RedditExtractoR" "RGtk2Extras" "Rtextrankr" "RYandexTranslate" [22] "TraMineRextras" "vcdExtra" "ycinterextra"

We actually use UpSetR::upset in the upsetSamples function so I don't think requireNamespace would work out unless we re-implement the upset algorithm in the package (unlikely).

We would have to move this feature into another package. The same would go with the shiny app.

Regards, Marcel

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/waldronlab/MultiAssayExperiment/issues/199#issuecomment-297777153, or mute the thread https://github.com/notifications/unsubscribe-auth/AAARIjH8QnXVos4NRsigTNUzMjo4oq7mks5r0MpmgaJpZM4NJiX2 .

ttriche commented 7 years ago

MultiAssayPlotter?

Put all the layout/mapping functionality into one place and use requireNamespace() to inform the user that it's time to install? This seems sensible and more or less mandates that the plots end up in their own vignette, which is a bonus of sorts. The name could maybe be more general, since the task (figure out how a ton of assays are related to one another) is common.

I learned several things (MAE has upset plots, requireNamespace() is an option to strongly suggest stuff, and tacking Extra onto a package name is in fact a bad idea) here. Thanks all

--t

On Thu, Apr 27, 2017 at 10:12 AM, Michael Lawrence <notifications@github.com

wrote:

An "Extras" package is a meaningless way to group functionality. Please organize stuff into meaningful modules.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/waldronlab/MultiAssayExperiment/issues/199#issuecomment-297779664, or mute the thread https://github.com/notifications/unsubscribe-auth/AAARIiYvi6_jZ-hjq9M2vqKyno-Sw9YEks5r0Mx2gaJpZM4NJiX2 .