metavizr

[hcorrada]

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

• Repository: https://github.com/epiviz/metavizr

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

• [ X] I understand that by submitting my package to Bioconductor, the package source and all review commentary are visible to the general public.
• [ X] I have read the Bioconductor Package Submission instructions. My package is consistent with the Bioconductor Package Guidelines.
• [ X] My package addresses statistical or bioinformatic issues related to the analysis and comprehension of high throughput genomic data.
• [ X] I am committed to the long-term maintenance of my package. This includes monitoring the support site for issues that users may have, subscribing to the bioc-devel mailing list to stay aware of developments in the Bioconductor community, responding promptly to requests for updates from the Core team in response to changes in R or underlying software.

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

• [ X] The 'devel' branch for new packages and features.
• [ X] The stable 'release' branch, made available every six months, for bug fixes.
• [ X] Bioconductor version control using Subversion (optionally via GitHub).

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

[bioc-issue-bot]

Hi @hcorrada

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: metavizr
Type: Package
Version: 0.99.0
Authors@R: c( person("Hector", "Corrada Bravo", email="hcorrada@gmail.com", role=c("cre","aut")),
  person("Florin", "Chelaru", email="florin.chelaru@gmail.com", role=c("aut")),
  person("Justin", "Wagner", email="wagner.justin1@gmail.com", role=c("aut")),
  person("Jayaram", "Kancherla", email="jayaram.kancherla@gmail.com", role=c("aut")),
  person("Jospen", "Paulson", email="nosson@gmail.com", role=c("aut")))
License: MIT + file LICENSE
Title: R Interface to the metaviz web app for interactive metagenomics data analysis and visualization
Description: This package provides Websocket communication to the
    metaviz web app (http://metaviz.cbcb.umd.edu) for interactive
    visualization of metagenomics data. Objects in R/bioc interactive
    sessions can be displayed in plots and data can be explored using a facetzoom visualization. Fundamental
    Bioconductor data structures are supported (e.g., MRexperiment objects),
    while providing an easy mechanism to support other data structures.
    Visualizations (using d3.js) can be easily added to the web app as well.
VignetteBuilder: knitr
Depends:
    R (>= 3.0.1),
    methods,
    Biobase
Imports:
    stringr,
    epivizr,
    epivizrData,
    metagenomeSeq,
    RNeo4j,
    S4Vectors,
    httpuv (>= 1.3.0)
Suggests:
    knitr,
    BiocStyle,
    matrixStats,
    msd16s
Collate:
    'startMetaviz.R'
    'utils.R'
    'Ptr-class.R'
    'MetavizNode-class.R'
    'MetavizTree-class.R'
    'EpivizMetagenomicsData-class.R'
    'register-methods.R'
    'plots.R'
biocViews: Visualization, Infrastructure, GUI
RoxygenNote: 5.0.1

[bioc-issue-bot]

Your package has been approved for building. Your package is now submitted to our queue.

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.

[bioc-issue-bot]

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 following build report for more details:

http://bioconductor.org/spb_reports/metavizr_buildreport_20160927093429.html

[lawremi]

Hi Hector,

Metaviz looks cool. Maybe the data should be in a separate package? The vignette seems shallow. It does not really motivate or explain the package. There would probably be some overlap with the Metaviz tutorial, but apparently that does not exist yet either. With reference classes, I would prefer making explicit reference to .self instead of using <<- to assign values to fields. Not sure if that's formal Bioc policy or not (yet). I also noticed that "stringr" is listed in Imports but NAMESPACE does not import it.

[bioc-issue-bot]

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 following build report for more details:

http://bioconductor.org/spb_reports/metavizr_buildreport_20160930103343.html

[lshep]

A more detailed review will not begin until ERRORs in the build report are corrected.

Any package that is imported in the NAMESPACE should be listed as import (or handled appropriately with Depends/suggests) in the DESCRIPTION file.

[lawremi]

The error in the build report is surprising, since epivizrStandalone is a Bioc package.

[lshep]

This should be resolved if they add epivizrStandalone to the import list in the DESCRIPTION file.

[bioc-issue-bot]

We only start builds when the Version field in the DESCRIPTION file is incremented. For example, by changing

Version: 0.99.0

to

Version 0.99.1

If you did not intend to start a build, you don't need to do anything. If you did want to start a build, increment the Version: field and try again.

[bioc-issue-bot]

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

58713cf version bump

[hcorrada]

Hi Mike and Lori,

This push should build without error. There is some documentation issues we need to address but they are listed as warnings and errors.

Here are some comments on Mike's review comments:

Metaviz looks cool.

Thanks!

Maybe the data should be in a separate package?

It's gone, we use example data from metagenomeSeq instead.

The vignette seems shallow. It does not really motivate or explain the package. There would probably be some overlap with the Metaviz tutorial, but apparently that does not exist yet either.

Agreed, we are working on it this week

With reference classes, I would prefer making explicit reference to .self instead of using <<- to assign values to fields. Not sure if that's formal Bioc policy or not (yet).

Not addressed yet in this last push, but will address in the next couple of days. We've adopted this style on our other pkgs, but there is some code here we need to fix.

I also noticed that "stringr" is listed in Imports but NAMESPACE does not import it.

Will fix.

[hcorrada]

• [x] Flesh out vignette
• [x] Fix .self instead of <<-
• [x] Fix dependency on stringr

[bioc-issue-bot]

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 following build report for more details:

http://bioconductor.org/spb_reports/metavizr_buildreport_20161001170235.html

[bioc-issue-bot]

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

2f226a8 deal with sd in metavizControl

[bioc-issue-bot]

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 following build report for more details:

http://bioconductor.org/spb_reports/metavizr_buildreport_20161001184740.html

[lawremi]

Hi Hector,

I'm going to help out the core crew with this submission. There are a lot of warnings and notes that seem easy to fix, so I will let you do that first before I proceed with a more formal review.

[bioc-issue-bot]

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

2f0e609 add params to write dump file (toNeo4jDump) dd0da99 validate MRExperiment object 002a685 add more biom format checks 47284cb remove space in cypher query for leaf_of relations... b03004c update getCombined request format handling ac7614c fix row start and end indices 657fc12 add params to write dump file (toNeo4jDump) 33b16b4 validate MRExperiment object 214ed8f add more biom format checks 6e44c1d remove space in cypher query for leaf_of relations... 850dff3 update getCombined request format handling 458e3a1 fix row start and end indices a540fae handle null ranges 215b228 rebase with master dd9a5cf vignette describes how to add heatmap and stackedl... fc0ae0f remove host from vignette 02ae6b4 remove merge issues 3705b85 add screenshot to vignette & update check biom for... a2c8a3f add screenshot 9538ddf Handle R CMD check warnigns and notes * edit vign... fd35f9a Code cleanup to handle BiocCheck warnings and note... f916f02 version bump

[hcorrada]

@lawremi

Hi Mike,

Thanks for helping out! We've done a few things on this new version

• expanded vignette, there is still more description of UI coming in the next day or so
• all warnings from R CMD check should now be gone, there is one warning from BiocCheck but per Martin's msg to bioc-devel yesterday, that should go away in a new version of BiocCheck

Let us know what you think.

[bioc-issue-bot]

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 following build report for more details:

http://bioconductor.org/spb_reports/metavizr_buildreport_20161007095617.html

[lawremi]

With respet to the warning, why not just importFrom(Biobase, fData)? Having dependencies listed in the NAMESPACE file is convenient, at least in my opinion. I can see how qualifying symbols can be helpful when resolving ambiguity, or using an unusal function (where did this come from?). But there's no ambiguity and everyone knows the origin of fData().

I'll start working on the review.

[bioc-issue-bot]

We only start builds when the Version field in the DESCRIPTION file is incremented. For example, by changing

Version: 0.99.0

to

Version 0.99.1

If you did not intend to start a build, you don't need to do anything. If you did want to start a build, increment the Version: field and try again.

[bioc-issue-bot]

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

76e5288 version bump

[hcorrada]

This takes care of the remaining warning. Thanks!

[bioc-issue-bot]

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 following build report for more details:

http://bioconductor.org/spb_reports/metavizr_buildreport_20161007144318.html

[lawremi]

Documentation

General

• Consider a non-trivial README.md to advertise the software in your GH repository. This could just link to the Bioc package landing page.

Vignette

• The vignette could still do a better job of describing the example data. While the data are described formally elsewhere, a few sentences about the type of object, what it contains, etc, would be helpful.
• What is all the plot type registration in the vignette? Is the user expected to copy-paste that?
• The construction of the metavizControl object could use some more explanation. It would to tell the reader the goal of the analysis, and how the parameters map to it. Are all of those parameters needed or can we rely on defaults? Could this first example be a bit simpler?
• Why is the heatmap constructed using several function calls instead of the simpler $plot()? Does $plot() not support a subset of a data source?
• Please demonstrate the high-level plotting functions.

Man pages

• How do I get an instance of the EpivizMetagenomicsData class? No mention in the man page.
• The method documentation often do not indicate the type of object returned. For example, what does get_measurements() return?
• The method documentation does not describe the arguments. What does colLabel do with toNEO4JDb()?
• The MetavizTree class is undocumented; should it be exported?

Dependencies

• Why only import new() from the methods package? Surely the package uses many more symbols from methods, like setMethod(), setClass(), etc. Best practice is to just import(methods). Typically, methods should also be in Depends.
• Perhaps metagenomeSeq also belongs in Depends for the sake of user convenience?

API

• Could the app$plot() method get the datasource_name by deparsing the first argument expression?
• The $add_measurements() method from epivzir should probably be called $create_datasource(), and a data source should be subsettable directly, like the measurements. The existence of both app$plot() and app$chart_mgr$visualize() is a little confusing and should be explained.
• What is the NEO4J dumping for? How is it related to visualization? Could move this somewhere else.

Implementation

• It is not at all clear why the tree implementation needs Ptr. There are many problems:

  1. The high-level type information is lost, since data structures are only storing the generic Ptr type.
  2. Much complexity stems from dereferencing and the potential for unexpected side effects.

  In any language, mutable state adds complexity and thus should be used sparingly and through a write barrier. In R this equates to putting fields inside of formal reference classes. It's not clear why the tree would not work by having reference objects as nodes (as now) without all the /Ptr/s, perhaps also with some sort of "context" object that is manipulated during recursion.

• Lots of sapply(), lapply() and for() calls. Could these be vectorized or changed to vapply()? Stuff like

  lapply(seq_along(leafInfos), function(i) { "<NA>" })

  should be vectorized, like

  rep(list("<NA>"), length(leafInfos))

  but maybe the subassignment will recycle automatically.

  Also consider AtomicList derivatives from IRanges and utilities from S4Vectors. This:

  sapply(leafAncestors, function(ancestors) { 
      paste(lapply(rev(ancestors), function(node) { node$id() }), collapse=",") 
    })

  Could be:

  unstrsplit(lapply(leafAncestors, function(ancestors) {
    vapply(rev(ancestors), function(node) node$id(), character(1L))
  }), ",")

• Probably should drop the MySQL backend as it appears to be unused and broken.

Misc

• Many of the R files have executable permissions, which is weird.

[hcorrada]

Thank you for the great review Mike. We will address these after this release round so metavizr is added to the devel branch (3.5). I will close the issue for now, and reopen in two weeks or so.

[hcorrada]

Hi @lawremi,

We are reopening this issue so metavizr is included in the next release. We have addressed most of the issues listed above, and will comment on them soon. I will first, push the newest version so building checks etc. are done. Once this issue is ready for your review again I will add a comment to let you know.

Thanks for the great initial feedback! Hector

[bioc-issue-bot]

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

76f97ab version bump

[bioc-issue-bot]

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 following build report for more details:

http://bioconductor.org/spb_reports/metavizr_buildreport_20170406133540.html

[bioc-issue-bot]

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

263bb4e update metagenomeSeq version dependency; version b... beb945c Merge branch 'master' of https://github.com/epiviz...

[bioc-issue-bot]

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 following build report for more details:

http://bioconductor.org/spb_reports/metavizr_buildreport_20170406135127.html

[bioc-issue-bot]

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

c1062b8 yavb

[bioc-issue-bot]

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 following build report for more details:

http://bioconductor.org/spb_reports/metavizr_buildreport_20170406135241.html

[hcorrada]

This error is addressed by this commit to metagenomeSeq: https://github.com/HCBravoLab/metagenomeSeq/commit/ccee76841cb94fa8f674948ca9146f871309c29b

It should clear when the build system updates metagenomeSeq

[jmwagner]

Hi @lawremi,

Thank you for the review and comments of the prior submission of metavizr. Below are the responses to the comments you provided. Please let us know as you have any additional questions/comments.

Documentation General

Consider a non-trivial README.md to advertise the software in your GH repository. This could just link to the Bioc package landing page.

The README.md has been updated with links to metaviz.org and tutorials.

Vignette

The vignette could still do a better job of describing the example data. While the data are described formally elsewhere, a few sentences about the type of object, what it contains, etc, would be helpful.

The vignette has been updated to include descriptions of all example data.

What is all the plot type registration in the vignette? Is the user expected to copy-paste that?

The plot type registration is not visible when building the vignette and we added a note the user. The plot type registration is required for the registering each chart type so that the vignette will build when starting a new Metaviz session using localhost.

The construction of the metavizControl object could use some more explanation. It would to tell the reader the goal of the analysis, and how the parameters map to it. Are all of those parameters needed or can we rely on defaults? Could this first example be a bit simpler?

We added an explanation of the metavizControl object creation and now specify one parameter while relying on defaults for the rest.

Why is the heatmap constructed using several function calls instead of the simpler $plot()? Does $plot() not support a subset of a data source?

We have updated the epivizr package to include a revisualize function. revisualize takes the chart object returned from $plot() and uses the measurments to create a new chart.

Please demonstrate the high-level plotting functions.

We take this to mean explaining what objects can plot take as input. We add the following sentence in the vignette before the plot call: "The plot function can take as input any of the classes registered with epivizrData."

Man pages

How do I get an instance of the EpivizMetagenomicsData class? No mention in the man page.

The documentation now contains an example for creating an instance of EpivizMetagenomicsData.

The method documentation often do not indicate the type of object returned. For example, what does get_measurements() return?

The method documentation for getMeasurements, getHierarchy, getRows, getValues, getCombined, and the MetavizGraph functions now includes description of return.

The method documentation does not describe the arguments. What does colLabel do with toNEO4JDb()?

The method documentation for getMeasurements, getHierarchy, getRows, getValues, getCombined, and the MetavizGraph functions now includes description of arguments. Specifically for toNEO4JDb(), the NEO4J export is being moved to a new class.

The MetavizTree class is undocumented; should it be exported?

The MetavizTree class has been removed and replaced by MetavizGraph. The MetavizGraph class has been documented and is not exported.

Dependencies

Why only import new() from the methods package? Surely the package uses many more symbols from methods, like setMethod(), setClass(), etc. Best practice is to just import(methods). Typically, methods should also be in Depends.

methods has been moved to Depends and the best practice of import(methods) is now followed.

Perhaps metagenomeSeq also belongs in Depends for the sake of user convenience?

metagenomeSeq has been moved to Depends.

API

Could the app$plot() method get the datasource_name by deparsing the first argument expression?

datasource_name is now deparsed from the data object. datasource_name is now an optional argument in case the user wants to specify a given name to appear in the Metaviz UI.

The $add_measurements() method from epivzir should probably be called $create_datasource(), and a data source should be subsettable directly, like the measurements. The existence of both app$plot() and app$chart_mgr$visualize() is a little confusing and should be explained.

We believe that both of these are epivizr issues and not specific to metavizr.

What is the NEO4J dumping for? How is it related to visualization? Could move this somewhere else.

The NEO4J dumping has been removed and a new class which will handle database export of the MRexperiment is being developed.

Implementation

It is not at all clear why the tree implementation needs Ptr. There are many problems:

1. The high-level type information is lost, since data structures are only storing the generic Ptr type.
2. Much complexity stems from dereferencing and the potential for unexpected side effects.

In any language, mutable state adds complexity and thus should be used sparingly and through a write barrier. In R this equates to putting fields inside of formal reference classes. It's not clear why the tree would not work by having reference objects as nodes (as now) without all the /Ptr/s, perhaps also with some sort of "context" object that is manipulated during recursion.

The Ptr class has been removed. The new implementation makes use of data.tables to store nodes and edges of the hierarchical feature data. The feature count values are also stored in a data.table. Aggregation queries for specified features are now computed using joins between the nodes, edges, and counts tables.

Lots of sapply(), lapply() and for() calls. Could these be vectorized or changed to vapply()? Stuff like lapply(seq_along(leafInfos), function(i) { "" }) should be vectorized, like rep(list(""), length(leafInfos)) but maybe the subassignment will recycle automatically. Also consider AtomicList derivatives from IRanges and utilities from S4Vectors. This: sapply(leafAncestors, function(ancestors) { paste(lapply(rev(ancestors), function(node) { node$id() }), collapse=",") }) Could be: unstrsplit(lapply(leafAncestors, function(ancestors) { vapply(rev(ancestors), function(node) node$id(), character(1L)) }), ",")

We are working to remove for() loops from this implementation and replace with vectorized calls.

Probably should drop the MySQL backend as it appears to be unused and broken.

The MySQL backend has been removed.

Misc

Many of the R files have executable permissions, which is weird.

The executable permissions on the R files have been removed.

[lawremi]

Thanks for addressing my comments. Even with the revisualize() convenience, the API does not seem very intuitive compared to grammar-based interfaces like ggplot2. Hopefully something like that could be wrapped around this in the future. I'm OK accepting this.

[hcorrada]

Thanks! Once an error-free build is done (should be at some point tomorrow, once metagenomeSeq is on the build system), I'll ping again to change the status of this package.

Thanks again!

• Yes, we are definitely overdue for an epivizr high-level API.

[mtmorgan]

I'll mark this as accepted, anticipating that the package will build successfully once metagenomeSeq's update propagates. Thanks for your help!