search

ncborcherding / escape

Easy single cell analysis platform for enrichment
https://www.borch.dev/uploads/screpertoire/articles/running_escape
MIT License
143 stars 20 forks source link

update GSVA function with new name #112

Closed kew24 closed 1 month ago

kew24 commented 1 month ago

As of 2 weeks ago (in https://github.com/rcastelo/GSVA/commit/03ad0efd197b470d9c0966e509236f945ebb224d), GSVA renamed their function compute.gene.density as compute.gene.cdf. I've updated the corresponding code in densityEnrichment.

Thanks for this package – super useful with some great visualizations!

kew24 commented 1 month ago

Not sure how this will work version-wise with GSVA – if users have older versions of GSVA, then compute.gene.cdf won't work for them. I can add some sort of error handling to try loading in compute.gene.cdf, and if that doesn't work, to load in compute.gene.density instead. Let me know if you want me to include that in this PR.

ncborcherding commented 1 month ago

Thanks for the pull request - you are right that the version is going to be a big issue here. Bioconductor 3.19 will be incompatible, which is a major limitation as that will be the standard pull for installation (or <3.19). I have the repo running unit tests as we speak to see.

We might put a pin in this until 3.20 release - let me get the other pull request taken care of and thanks again.

kew24 commented 1 month ago

No worries – thanks for looking into compatibility here. For now, I'll just use the modded version in my fork. Wanted to be sure to give you a heads up about what I encountered. Feel free to close or edit my PR if you find a different solution.

Thanks again!

ncborcherding commented 1 month ago

@kew24

Rethinking this - could you make the pull request to the "dev" branch? That will be the branch I use in the upcoming bioconductor 3.20 version. So for anyone with the updated GSVA, they can install the dev branch for now.

Also if interested please feel free to add your name to the function (as @author) or the package DESCRIPTION.

Thanks, Nick

kew24 commented 1 month ago

Done. Just switched the branch to dev. With such a minor contribution, I don't think I need to list myself on the function or package, but if I make a more worthy PR (or multiple smaller ones) in the future, I certainly will. Thanks!

kew24 commented 1 month ago

Unrelated to the GSVA function name at hand, but I also realized your vignette didn't render a header properly because of a spacing issue next to a code block. Would you want me to add this tweak to this PR, or create a separate tiny one? (see https://github.com/kew24/escape/commit/ebad6ab653e81957ae87690f36c6a8c71bc2fb2d)

ncborcherding commented 1 month ago

Great thanks a ton - I will set a GSVA version requirement for the dev branch and it should help other users during the interim period. Also thanks for spying the vignette issue - I'll get it fixed!!