Open PeteHaitch opened 4 years ago
I decided to reduce the available ties.methods
not out of necessity, but to ease extension.
For reference, base::ranks()
supports:
c("average", "first", "last", "random", "max", "min")
and matrixStats::rowRanks()
supports:
c("max", "average", "first", "last", "random", "max", "min", "dense")
I felt that the it could be an undue burden to ask each extension to implement 7 different ties.methods
. Especially, if base R only supports 6.
Of course each extension of MatrixGenerics
could just give an error if a ties.methods
is not supported, but this could lead to package developers avoiding the rowRanks()
function, as it might break for different matrix types.
I thought that it was a good compromise to select a reduced number of ties.methods
. This makes it more likely that every extension supports this reduced number of ties.methods
and makes the method more reliable for users of the MatrixGenerics
package. In addition, I decided that it is easier to later add ties.method
's than to take on away from the API.
I decided to pick max
and average
mainly because those where the two that I had already implemented in sparseMatrixStats
. The sparse structure means that it is tedious to implement for example ties.method = "first"
. However, I realize this is a subjective judgement and I am looking forward to your take which ties.method
's you would put into the API?
Why was this necessary @const-ae ?