DelayedMatrixStats icon indicating copy to clipboard operation
DelayedMatrixStats copied to clipboard

Implement `useNames` argument

Open PeteHaitch opened this issue 3 years ago • 9 comments

Following changes made to MatrixGenerics in https://github.com/Bioconductor/MatrixGenerics/issues/23, need to update client packages such as DelayedMatrixStats. Process will be something like:

  • [ ] Add unit tests; something like https://github.com/HenrikBengtsson/matrixStats/blob/ff261339136ec7c5cb03a882449a50325f57c159/tests/rowCollapse.R#L16-L28
  • [ ] Add useNames = TRUE and useNames = FALSE behaviour; something like https://github.com/HenrikBengtsson/matrixStats/blob/ff261339136ec7c5cb03a882449a50325f57c159/R/rowAlls.R#L96-L114 to all functions with useNames argument. Note that the code varies depending on whether it is a row- or col-wise function and possibly on whether the current method returns names or does not.

See also https://github.com/const-ae/sparseMatrixStats/commit/eaab67410f5c630ea9cec8900c3f62287a0d630d

PeteHaitch avatar Aug 03 '21 22:08 PeteHaitch

Hi Pete, our Bioc2021 workshop package started failing as DelayedMatrixStats won't install which is then causing a few other packages to not install. I think it's this useNames issue?

https://github.com/stemangiola/bioc2021_tidytranscriptomics/runs/3243242486?check_suite_focus=true#step:8:9871

ERROR: lazy loading failed for package ‘DelayedMatrixStats’
Error : in method for ‘colAlls’ with signature ‘x="DelayedMatrix"’:  arguments (‘useNames’) after ‘...’ in the generic must appear in the method, in the same place at the end of the argument list
Error: unable to load R code in package ‘DelayedMatrixStats’
Execution halted
* removing ‘/usr/local/lib/R/site-library/DelayedMatrixStats’

I'm not sure how to fix this so wondering if you happen to know of any temporary workaround?

mblue9 avatar Aug 04 '21 16:08 mblue9

Can you roll back to an earlier Docker image?

Sorry, I'm also attending BioC and so don't have much time to work on this. If I manage to implement a workaround it won't be available through BiocManager::install() for a day or 2 but may be available via GitHub. I'm not sure if that's helpful to you?

PeteHaitch avatar Aug 04 '21 22:08 PeteHaitch

I've made a quick fix on GitHub

  • If you are using the current BioC release (3.13) for your workshop, you should be able to use 906ef69596558b0b7436ce95ad81a529062fa38c via BiocManager::install("PeteHaitch/DelayedMatrixStats@906ef69596558b0b7436ce95ad81a529062fa38c")
  • If you are using the current BioC devel (3.14) for your workshop, you should be able to use 949d3868d26a63861abe6109e18e62f352c0c41e via BiocManager::install("PeteHaitch/DelayedMatrixStats@949d3868d26a63861abe6109e18e62f352c0c41e")

As I said, these won't be available for a few days from BioC itself (rather than installing from GitHub) once its passes BioC's check and build process.

This is only step 1 of properly supporting useNames and there's still more careful work required, but I hope this change will allow you and others to install packages that depend on DelayedMatrixStats.

PeteHaitch avatar Aug 05 '21 01:08 PeteHaitch

Having the same issue...

hisplan avatar Aug 05 '21 03:08 hisplan

Thanks @PeteHaitch ! We were going to roll back to an earlier image if we had to but your fix here seems to have done the trick. We're using BioC devel (3.14) and adding this worked BiocManager::install("PeteHaitch/DelayedMatrixStats@949d3868d26a63861abe6109e18e62f352c0c41e")

Thanks a lot for the quick fix, really appreciate it!!

mblue9 avatar Aug 05 '21 08:08 mblue9

Worked for me as well (Bioc 3.13). Thanks!

Though, I'm a bit confused because it appears to me that none of the dependencies has changed (maybe I'm missing something here), yet I was getting that error when trying to build the same docker image. I was able to build the same image just fine a month ago... Any idea??

hisplan avatar Aug 05 '21 11:08 hisplan

@hisplan I'm not sure I quite understand your question but I'll do my best to answer.

In the past month some changes in matrixStats necessitated changes in MatrixStats which in turn necessitated changes in DelayedMatrixStats (and sparseMatrixStats) ... So a month ago these dependencies were all in-sync and compatible but then during that period they fell out-of-sync which caused this recent problem. Now they're back in-sync and so things should work again.

PeteHaitch avatar Aug 05 '21 11:08 PeteHaitch

I see. You said things should work again, but I'm still not able to build my docker image unless I use the temporary solution you provided earlier... The package that I'm installing is called scCB2 (https://bioconductor.org/packages/release/bioc/html/scCB2.html)

hisplan avatar Aug 05 '21 13:08 hisplan

As I said in https://github.com/PeteHaitch/DelayedMatrixStats/issues/84#issuecomment-893101166, it'll be a day or two for these changes to propagate to BioC and then everything will be in sync when installing from BioC with BiocManager::install(). While we wait for those changes to propagate we have to use this temporary workaround

PeteHaitch avatar Aug 05 '21 21:08 PeteHaitch