Message-ID: <5918c61f0612141257i2e0c4ffdl387c1b854a37ed23@mail.gmail.com>
Date: 2006-12-14T20:57:41Z
From: John Zedlewski
Subject: max.col: bug or just oddity?
I've noticed that the max.col function with the default "random"
option often gives unexpected results. For instance, in this test, it
seems clear what the answer should be:
> # second col should always be max
> x1 = cbind(1:10, 2:11, -Inf)
>
> # this works fine
> max.col(x1, "first")
[1] 2 2 2 2 2 2 2 2 2 2
>
> # this gives random answers
> max.col(x1)
> [1] 3 1 1 2 3 3 1 3 1 1
Ouch! max.col is randomizing across all values.
Even without infinite values, something similar can happen:
> # test 2 --- tolerance problems
>
> # clearly column 3 is the max
> x1 = cbind(-1e9 * 1:10, 1:10, 2:11)
>
> # again, first method works:
> max.col(x1, "first")
[1] 3 3 3 3 3 3 3 3 3 3
>
> # but random doesn't
> max.col(x1)
[1] 2 3 2 3 3 2 2 2 3 2
>
The max.col docs say " there is a relative tolerance of 1e-5, relative
to the largest entry in the row", but it's really using the maximum
absolute value entry in the row (appl/maxcol.c, line 35 in R 2.4.0).
Is this necessary for some sort of S-plus compatibility? If so, I
think it would be good to make this absolute value property very clear
in the docs, since it can cause subtle bugs (and did for me).
Personally, I think the behavior is much nicer with the following patch:
--- rplain/R-2.4.0/src/appl/maxcol.c 2006-04-09 18:19:58.000000000 -0400
+++ R-2.4.0/src/appl/maxcol.c 2006-12-14 15:30:56.000000000 -0500
@@ -26,13 +26,14 @@
do_rand = *ties_meth == 1;
for (r = 0; r < n_r; r++) {
- /* first check row for any NAs and find the largest abs(entry) */
+ /* first check row for any NAs and find the largest entry */
large = 0.0;
isna = FALSE;
for (c = 0; c < *nc; c++) {
a = matrix[r + c * n_r];
if (ISNAN(a)) { isna = TRUE; break; }
- if (do_rand) large = fmax2(large, fabs(a));
+ if (!R_FINITE(a)) continue;
+ if (do_rand) large = fmax2(large, a);
}
if (isna) { maxes[r] = NA_INTEGER; continue; }
--------------------------------------- END
-------------------------------------------------
This gives the expected behavior in the two examples above.