hansenlab / mpra

5 stars 5 forks source link

add block_vector to normalization call #8

Closed pi-zz-a closed 2 months ago

pi-zz-a commented 2 months ago

The block vector was not passed as an argument to the normalize_counts function call. This caused incorrect normalization.

pi-zz-a commented 2 months ago

Closes #9

mikelove commented 2 months ago

I have an error building the package:

Quitting from lines ... [unnamed-chunk-9] (mpra.Rmd)
Error: processing vignette 'mpra.Rmd' failed with diagnostics:
object 'block' not found
--- failed re-building ‘mpra.Rmd’

I was trying the vignette when the fit is run with model_type = "indep_groups" and block not specified.

This calls .fit_standard which then calls normalize_counts(object, block).

I think a fix is to explicitly list block=NULL as an argument for .fit_standard. If that sounds right to @pi-zz-a and @lmyint I will do a PR.

lmyint commented 2 months ago

In .fit_standard the normalize_counts(object, block) should have just been normalize_counts(object) so that block = NULL would be used by default. I fixed this after @pi-zz-a 's PR in devel and release but didn't update the master branch--just fixed that now.

mikelove commented 2 months ago

Oh thanks! I'll switch to devel branch

mikelove commented 2 months ago

@lmyint small suggestion: could you make devel branch the default branch for your github? that's what I do with my Bioc packages.

And just to give a preview: I'm going to work on a smallish PR that adds an option for mpralm() to return an MPRASet with the limma-voom results attached in whole to the object and as columns to rowData. I will use a strategy that has been used before in Bioc, e.g. rotation matrices are added by scran to a SingleCellExperiment (in the attributes) or quant details are added to SummarizedExperiment by tximeta (in the metadata).

mikelove commented 2 months ago

Forking on GitHub forks from whatever the default branch is, so you want people working with whatever is the leading edge.

Screenshot 2024-08-21 at 10 53 34 AM

lmyint commented 2 months ago

Thanks for the suggestion @mikelove -- just updated the default branch to devel. And that's a useful feature, thanks for working on it!