alyssafrazee / polyester

Bioconductor package "polyester", devel version. RNA-seq read simulator.
http://biorxiv.org/content/early/2014/12/12/006015
90 stars 51 forks source link

`simulate_experiment` does not accept a list of numerics for `gcbias` #44

Closed javiljoen closed 7 years ago

javiljoen commented 7 years ago

Hello! I've come across an issue using the gcbias parameter of simulate_experiment in Polyester v. 1.10.0 (the latest version in Bioconductor 3.4). When I pass in a list of numerics of length sum(num_reps), as per the description in the help file, the following error is thrown:

Error in polyester::simulate_experiment(fastapath, gcbias=as.list(rep(1, sum(num_reps))), : 
  gc bias parameters must be integers 0 through 7 or loess objects

This seems to be caused by the application of unique() to the vector of classes in L419 https://github.com/Bioconductor-mirror/polyester/blob/master/R/simulate_experiment.R#L417-L423:

if('gcbias' %in% names(extras)){
    stopifnot(length(extras$gcbias) == sum(num_reps))
    gcclasses = unique(sapply(extras$gcbias, class))
    if(sum(gcclasses %in% c('numeric', 'loess')) < length(extras$gcbias)){
        stop(.makepretty('gc bias parameters must be integers 0 through 7
            or loess objects'))
    }

For example

> num_reps = c(2, 2)
> gcbias = as.list(rep(1, 4))
> gcclasses = unique(sapply(gcbias, class))
> gcclasses
[1] "numeric"
> length(gcbias)
[1] 4
> sum(gcclasses %in% c('numeric', 'loess')) < length(gcbias)
[1] TRUE

Changing L419 to

gcclasses = sapply(extras$gcbias, class)

would result in sum(gcclasses %in% c('numeric', 'loess')) == length(extras$gcbias) if all elements of gcbias are either "numeric" or "loess".

(The test could then equivalently be stated as:

if(! all(sapply(extras$gcbias, class) %in% c("numeric", "loess"))) { stop(...) }

)

Have I misunderstood the format the gcbias parameter is supposed to take?

alyssafrazee commented 7 years ago

Hey Jack, thanks for the bug report! Looks like you're exactly right.

I won't be able to get to this until Monday at the earliest. If you file a pull request, I'm happy to merge it in, or I can take the fix next week. Apologies for the inconvenience but thanks for taking the time to ID the bug in such detail!

On Fri, Feb 3, 2017 at 8:39 AM, Jack Viljoen notifications@github.com wrote:

Hello! I've come across an issue using the gcbias parameter of simulate_experiment in Polyester v. 1.10.0 (the latest version in Bioconductor 3.4). When I pass in a list of numerics of length sum(num_reps), as per the description in the help file, the following error is thrown:

Error in polyester::simulate_experiment(fastapath, gcbias=as.list(rep(1, sum(num_reps))), : gc bias parameters must be integers 0 through 7 or loess objects

This seems to be caused by the application of unique() to the vector of classes in L419 https://github.com/Bioconductor-mirror/polyester/blob/master/R/simulate_ experiment.R#L417-L423:

if('gcbias' %in% names(extras)){ stopifnot(length(extras$gcbias) == sum(num_reps)) gcclasses = unique(sapply(extras$gcbias, class)) if(sum(gcclasses %in% c('numeric', 'loess')) < length(extras$gcbias)){ stop(.makepretty('gc bias parameters must be integers 0 through 7 or loess objects')) }

For example

num_reps = c(2, 2)> gcbias = as.list(rep(1, 4))> gcclasses = unique(sapply(gcbias, class))> gcclasses [1] "numeric"> length(gcbias) [1] 4> sum(gcclasses %in% c('numeric', 'loess')) < length(gcbias) [1] TRUE

Changing L419 to

gcclasses = sapply(extras$gcbias, class)

would result in sum(gcclasses %in% c('numeric', 'loess')) == length(extras$gcbias) if all elements of gcbias are either "numeric" or "loess".

(The test could then equivalently be stated as:

if(! all(sapply(extras$gcbias, class) %in% c("numeric", "loess"))) { stop(...) }

)

Have I misunderstood the format the gcbias parameter is supposed to take?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/alyssafrazee/polyester/issues/44, or mute the thread https://github.com/notifications/unsubscribe-auth/AB6e2B-kYH4jzlqfi-OjnQuhlA4Qp6Abks5rY1hbgaJpZM4L2iff .

javiljoen commented 7 years ago

Pull request submitted :)