search

MarioniLab / DropletUtils

Clone of the Bioconductor repository for the DropletUtils package.
https://bioconductor.org/packages/devel/bioc/html/DropletUtils.html
54 stars 25 forks source link

as(<dgTMatrix>, "dgCMatrix") is deprecated #98

Closed lambdamoses closed 1 year ago

lambdamoses commented 1 year ago

I'm an author of a package that uses read10xCounts() to read 10X Space Ranger data. I have been encountering the warning

as(<dgTMatrix>, "dgCMatrix") is deprecated since Matrix 1.5-0; do as(., "CsparseMatrix") instead

which comes from this line: https://github.com/MarioniLab/DropletUtils/blob/490715091ef56f24b354537b897aeeada6e7ffbf/R/read10xCounts.R#L275

Please update your code for the new version of Matrix. Thanks!

jonathangriffiths commented 1 year ago

Thanks, this one slipped through when I made a wider set of changes to address this Matrix change.

I plan to move the 10X-loaders out of this package to be replaced by TENxIO when I find the time - if you'd like to get ahead of that then you could consider developing around TENxIO already.

lambdamoses commented 1 year ago

Should the functions reading Space Ranger output be moved into TENxIO as well? Like SpatialExperiment::read10xVisium. I'm the main author of SpatialFeatureExperiment (SFE), which has read10xVisiumSFE where I got this warning. I suppose SpatialExperiment is more mature, while SFE is still experimental.

jonathangriffiths commented 1 year ago

Good question (sorry for the delay - I am back from a trip).

I think I'd have to leave it to your judgement as to whether TENxIO will be updated rapidly enough for your work - we do not all have the productivity of Aaron Lun! Certainly it does not seem to have any support for Visium yet, but it's not clear to me whether that will be the case permanently.

jonathangriffiths commented 1 year ago

Should be resolved in 72195b3cedaf44a5dabd5da9f5020258acc77a31

It is very confusing to me why the Matrix team specifically preserved as(x, "dgCMatrix") in 1.4-2 only to swap it over now.

Thanks for the tip-off.

jashapiro commented 1 year ago

I see that this has been fixed for a while, and of course it is only a deprecation warning, but I was wondering if there were any plans to push this as a bug fix into release 3.16? I'm not really sure what that might entail, or whether this kind of thing is done often, but I thought I might as well ask.

jonathangriffiths commented 1 year ago

I had planned to make this change only in devel really, which will release as Bioc 3.17 in a month. Is there any substantial upside for you if I move this change into the release version sooner?

I consider the RELEASE versions of bioconductor to be fixed except when there are actual bugs in the code - this deprecation warning has been caused by updates to a separate package, so doesn't fit in with that. I believe that is the Bioconductor "style" but I'm happy to be corrected on that.

jashapiro commented 1 year ago

That seems fine to me, all things considered. I just happen to be about to freeze an environment, and not having the warning would be a "nice to have" but absolutely not something I would push for. I tend to agree with your interpretation of what a RELEASE should mean, but I'm not always sure what the Bioc standard really is.