search

kharchenkolab / cacoa

Single-cell Case Control Analysis
46 stars 7 forks source link

clean up for CRAN #23

Closed evanbiederstedt closed 2 years ago

evanbiederstedt commented 2 years ago

Hi @VPetukhov

A few changes for CRAN checks

I noticed that there are two functions estimateExpressionShiftMagnitudes() which are both exported. This is probably wrong, and relates to the GitHub issues. Please check this.

If it's a Seurat issue, we can decide whether to support or ignore.

Otherwise, we're close. We'll need to add details to the roxygen2 documentation; I'd hope @shenglinmei @iganna @rrydbirk could help out.

VPetukhov commented 2 years ago

Hi Evan,

Thanks!

I noticed that there are two functions estimateExpressionShiftMagnitudes() which are both exported

Do you mean that our class method has the same name as its back-end function? Given that one is R6 method and another is not, I don't think it can lead to a conflict. Or what issues do you mean?

evanbiederstedt commented 2 years ago

Hi @VPetukhov

Do you mean that our class method has the same name as its back-end function?

Yep, that's what I mean.

Given that one is R6 method and another is not, I don't think it can lead to a conflict. Or what issues do you mean?

I think it's associated with roxygen2: R CMD CHECK --as-cran gives me problems. CRAN interprets this as an error, and will reject the package.

You're trying to use something akin to function overloading here? Is it useful? (Is it necessary?)

I think our options are to:

(1) Explicitly mark these two functions as the same with @ rdname so there aren't complaints (2) Rename the function in expression_distance.R to be something like estimateExpressionShiftMagnitudes2()

VPetukhov commented 2 years ago

@evanbiederstedt , sorry for dropping the ball here. I merged the cran_fixes branch into dev and renamed the inner estimateExpressionShiftMagnitudes to estimateExpressionChange.