Skip to content
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

Enable velocyto.R support both hdf5r and h5 #43

Merged
merged 8 commits into from Nov 23, 2018

Conversation

Puriney
Copy link
Contributor

@Puriney Puriney commented Oct 16, 2018

Motivation
The installation of velocyto.R cannot work mainly due to one of the dependencies, h5; this was also reported in these issues #42, #30, and #13 (#13 (comment)).

Here Yun enables the velocyto.R to support both hdf5r and h5, keeps the results consistent and avoids messing up the original code structure. In addition, this PR leaves flexibility for the author to decide if to keep h5 or exclusively to use hdf5r.

What's Changed

  1. As suggested by the author (Error installing velocyto.R #30 (comment)), the two affected functions read.loom.matrices() and read.gene.mapping.info() were fixed so that now they support both the hdf5r and h5. In details, they are both added one parameter, named engine, to select either 'hdf5r' or 'h5'. Default value is 'hdf5r'.
  2. In the DESCRIPTION file, 'h5' was changed to 'Suggests' section and 'hdf5r' was changed to 'Imports' section, because the h5 package is officially deprecated in favor of the hdf5r package. (See notice here).
  3. The 'man/*.Rd' manuals were automatically generated and so updated by roxygen2. The contents were consistent except that one error existed in 'man/find.ip.sites.Rd' was fixed. roxygen2 picked up a deprecated function (i.e., t.generate.ip.mask) and replaced it with the correct one (i.e., find.ip.sites).

Repeatable Demo
Use the official tutorial data to run the repeatable demo. The results were same and consistent.

  1. read.loom.matrices()
# 10X43_1.loom from: http://pklab.med.harvard.edu/velocyto/DG1/10X43_1.loom
fpath_loom <- 'data/10X43_1.loom'
ldat_h5 <- read.loom.matrices(fpath_loom, engine = 'h5')
ldat_hdf5r <- read.loom.matrices(fpath_loom, engine = 'hdf5r')
all.equal(ldat_h5, ldat_hdf5r)
# TRUE
  1. read.gene.mapping.info()
library(parallel)
# http://pklab.med.harvard.edu/velocyto/notebooks/R/chromaffin2.nb.html
ip.mm10 <- readRDS(url("http://pklab.med.harvard.edu/velocyto/chromaffin/ip.mm10.rds"))
gene.info <- read.gene.mapping.info(
  "data/velocyto/dump/onefilepercell_A1_unique_and_others_J2CH1.hdf5",
  internal.priming.info=ip.mm10,
  min.exon.count=10,
  engine='h5')
gene.info2 <- read.gene.mapping.info(
  "data/velocyto/dump/onefilepercell_A1_unique_and_others_J2CH1.hdf5",
  internal.priming.info=ip.mm10,
  min.exon.count=10,
  engine='hdf5r')

all.equal(gene.info, gene.info2)
# [1] "Component “info”: Component “is_intron”: 'current' is not a factor"    
# [2] "Component “info”: Component “is_last3prime”: 'current' is not a factor"
# [3] "Component “info”: Component “strandplus”: 'current' is not a factor" 

all.equal(gene.info$gene.df, gene.info2$gene.df)
# TRUE

for (x in names(gene.info$info)){
  cat(x, ', ')
  cat(all.equal(gene.info[[x]], gene.info2[[x]]))
  cat('\n')
}
# chrm , TRUE
# exino , TRUE
# features_gene , TRUE
# is_intron , TRUE
# is_last3prime , TRUE
# start_end , TRUE
# strandplus , TRUE
# tr_id , TRUE
# cluster.feature.counts , TRUE

Tested Env

  • Both the h5 and hdf5r can be installed
    • R version 3.4.2
    • x86_64-apple-darwin15.6.0 (64-bit)
  • Version if this PR were merged is successfully installed
    • R version 3.5.1
    • x86_64-apple-darwin15.6.0 (64-bit)

Thanks @alexbarrera for following up my issue.

@@ -33,6 +33,6 @@ annotated intronic regions
\examples{
\dontrun{
library(BSgenome.Mmusculus.UCSC.mm10)
ip.mm10 <- t.generate.ip.mask('refdata-cellranger-mm10-1.2.0/genes/genes.gtf',Mmusculus,'mm10')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roxygen2 picks up a deprecated function (i.e., t.generate.ip.mask) and replace it with the correct one (i.e., find.ip.sites).

…ions.

Revert ":pill: weaken the h5 dependency and document package by roxygen2"

This reverts commit 8a05044.
@mgood2
Copy link

mgood2 commented Oct 25, 2018

I get this error msg.

Error: package or namespace load failed for ??hdf5r?? in dyn.load(file, DLLpath = DLLpath, ...):
 unable to load shared object '/home3/elee4472/R/x86_64-pc-linux-gnu-library/3.5/hdf5r/libs/hdf5r.so':
  libhdf5_hl.so.100: cannot open shared object file: No such file or directory
Error: loading failed
Execution halted
ERROR: loading failed

@Puriney
Copy link
Contributor Author

Puriney commented Oct 25, 2018

I get this error msg.

Error: package or namespace load failed for ??hdf5r?? in dyn.load(file, DLLpath = DLLpath, ...):
 unable to load shared object '/home3/elee4472/R/x86_64-pc-linux-gnu-library/3.5/hdf5r/libs/hdf5r.so':
  libhdf5_hl.so.100: cannot open shared object file: No such file or directory
Error: loading failed
Execution halted
ERROR: loading failed

The error is all about the installation of hdf5r itself. The exact same issue/problem was reported here hhoeflin/hdf5r#106. Once you have hdf5r installed, you are probably ready to install and use velocyto.R.

@wresch wresch mentioned this pull request Nov 12, 2018
@pkharchenko pkharchenko changed the base branch from master to devel November 23, 2018 09:57
@pkharchenko pkharchenko merged commit 666e1db into velocyto-team:devel Nov 23, 2018
@pkharchenko
Copy link
Contributor

Got the following warning messages in the first test:

> ldat_hdf5r <- read.loom.matrices(fpath_loom, engine = 'hdf5r')
reading loom file via hdf5r...
[1] "Couldn't delete        144115188075855873"
[1] "Couldn't delete        144115188075855875"
[1] "Couldn't delete        144115188075855878"
[1] "Couldn't delete        144115188075855880"
[1] "Couldn't delete        144115188075855882"
> all.equal(ldat_h5, ldat_hdf5r)
[1] TRUE

The result is OK though.

@pkharchenko
Copy link
Contributor

OK, merged in to dev, tested, and merged all into master. Thank you very much for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants