Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
HenrikBengtsson opened this issue Jun 23, 2021 · 1 comment
Open

Comments

@HenrikBengtsson
Copy link

HenrikBengtsson commented Jun 23, 2021

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. HenrikBengtsson/matrixStats#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

@PeteHaitch
Copy link
Owner

@HenrikBengtsson your recent-ish updates (HenrikBengtsson/matrixStats#142 (comment) and HenrikBengtsson/matrixStats#142 (comment)) 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants