DelayedMatrixStats icon indicating copy to clipboard operation
DelayedMatrixStats copied to clipboard

HEADS UP: New default for `ties.method` of {col,row}Ranks()

Open HenrikBengtsson opened this issue 4 years ago • 1 comments

Background

matrixStats uses ties.method = "max" as the default for colRanks() and rowRanks() for legacy reasons, but we want eventually update to ties.method = "average" to align it with base::rank(), cf. https://github.com/HenrikBengtsson/matrixStats/issues/142.

The process for this migration with be:

  1. Give a deprecation warning if ties.method is not explicitly specified (long time; several releases)
  2. Give a defunct error if ties.method is not explicitly specified (long time; several releases)
  3. Switch the new default to ties.method = "average"

This will have to take a long time in order to make sure end-users out there will notice this and update their code. I hope this will minimize the risk for existing code all of a sudden start producing different results.

Issue

DelayedMatrixStats gives an ERROR when I revdep check asserting !isTRUE(missing(ties.method)), cf. https://github.com/HenrikBengtsson/matrixStats/blob/feature/default-rank-ties.method/revdep/R_MATRIXSTATS_TIES_METHOD_MISSING%3Ddefunct/problems.md#delayedmatrixstats

HenrikBengtsson avatar Jun 23 '21 16:06 HenrikBengtsson

@HenrikBengtsson your recent-ish updates (https://github.com/HenrikBengtsson/matrixStats/issues/142#issuecomment-2047669361 and https://github.com/HenrikBengtsson/matrixStats/issues/142#issuecomment-2049738860) prompted me to look at this again. But I can't see where DelayedMatrixStats (or MatrixGenerics) is not explicitly specifying ties.method.

This is what I ran locally with an up-to-date BioC devel:

R_MATRIXSTATS_TIES_METHOD_MISSING=defunct R_MATRIXSTATS_TIES_METHOD_FREQ=1 R CMD build .
R_MATRIXSTATS_TIES_METHOD_MISSING=defunct R_MATRIXSTATS_TIES_METHOD_FREQ=1 R CMD check DelayedMatrixStats_1.27.1.tar.gz

I didn't see any output that suggested any problems related to the ties.method argument.

Does that indicate the issue no longer exists for DelayedMatrixStats? Or are you able to help me reproduce the issue locally

PeteHaitch avatar May 20 '24 23:05 PeteHaitch