drisso / zinbwave

Clone of the Bioconductor repository for the zinbwave package, see https://bioconductor.org/packages/zinbwave

43 stars 10 forks source link

added 0 element in nleft; this way correct columns are removed from n… #49

Closed SzMK closed 4 years ago

SzMK commented 4 years ago

…ewfit while creation of newW object for zeroinflation==FALSE case and rbind does not stop

codecov-io commented 4 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@7083074). Click here to learn what that means. The diff coverage is 0%.

@@            Coverage Diff            @@
##             master      #49   +/-   ##
=========================================
  Coverage          ?   62.66%           
=========================================
  Files             ?       10           
  Lines             ?     1457           
  Branches          ?        0           
=========================================
  Hits              ?      913           
  Misses            ?      544           
  Partials          ?        0

Impacted Files	Coverage Δ
R/zinbsurf.R	`59.8% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7083074...ca71fb7. Read the comment docs.

drisso commented 4 years ago

First of all, sorry for waiting so long to react to this. I'm a bit confused as to why the current code is a problem and what this PR solves. Can you please elaborate on the error that you get without adding the 0 in nleft?

A reproducible example would be great.

A unit test would be even greater :)

Thanks! Davide

SzMK commented 4 years ago

Hi, I am not sure that I can now provide a reproducible example, but I think I might explain what's the problem. Please compare lines 156-158 of zinbsurf for zeroinflation != 0:

              nleft <- c(length(getEpsilon_gamma_mu(fit)),
                         length(getEpsilon_gamma_pi(fit)),
                         length(getEpsilon_W(fit)))

with lines 177-178 for zeroinflation == 0:

              nleft <- c(length(getEpsilon_gamma_mu(fit)),
                         length(getEpsilon_W(fit)))

Later, in line 192, to get matrix corresponding to W the following is done:

             newW <- t(newfit[-(seq_len(sum(nleft[1:2]))), ,drop = FALSE])

and then sum(nleft[1:2]) means different things for zeroinflation == 0 and zeroinflation != 0.

drisso commented 4 years ago

Oh, I see what you mean now, good catch!