From murdoch at stats.uwo.ca  Sat Dec 16 18:45:40 2006
From: murdoch at stats.uwo.ca (Duncan Murdoch)
Date: Sat, 16 Dec 2006 12:45:40 -0500
Subject: [Rd] max.col oddity
In-Reply-To: <5918c61f0612160925q3ab5b17cw6dadaaf0e033abfc@mail.gmail.com>
References: <5918c61f0612151609v5beb89c1ub02e40b2c7691bf0@mail.gmail.com>	<4583DA76.6010701@stats.uwo.ca>
	<5918c61f0612160925q3ab5b17cw6dadaaf0e033abfc@mail.gmail.com>
Message-ID: <458430C4.3050704@stats.uwo.ca>

On 12/16/2006 12:25 PM, John Zedlewski wrote:
> Duncan--
>  Ah, good point, clearly setting the tolerance to 0 is bad in that
> case. Also, my code has another problem when the max is negative -- it
> will set a negative tolerance! One good fix for both problems is the
> following: set the initial value of "large" to the first value in the
> row instead of 0.0, then keep the "fmax2(a,large)" approach from my
> patch, but at the end, take the absolute value of large. That will
> always follow the current docs -- using the largest value, not the
> largest absolute value in the row, for comparison.
> 
>   You proposal of changing the docs and just fixing the infinite
> problem sounds like a reasonable approach too, especially if people
> are already depending on this behavior (??), although I still think
> it's s a little weird that comparing "(-1e10, 2, 3)" will say that 2
> or 3 could be the max.

I think the explanation for this is in the man page:  "In this case, the 
determination of "tie" assumes that the entries are probabilities..." 
If you transform that vector linearly to force it into the [0,1] range 
of probabilities, the behaviour is clear:

 > x <- c(-1e10, 2, 3)
 > (x - min(x))/diff(range(x))
[1] 0 1 1

Duncan Murdoch

>  Thanks,
> --JRZ
> 
>> I think I'd prefer to be clear about what "largest" means in the docs
>> rather than dropping the absolute value, because if all entries are
>> negative, your version sets the tolerance to 0.
> 
> 
> On 12/16/06, Duncan Murdoch <murdoch at stats.uwo.ca> wrote:
>> On 12/15/2006 7:09 PM, John Zedlewski wrote:
>>> 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.
>> I think I'd prefer to be clear about what "largest" means in the docs
>> rather than dropping the absolute value, because if all entries are
>> negative, your version sets the tolerance to 0.
>>
>> But ignoring infinite values when calculating the largest absolute value
>> might be a good idea:  I can't see why someone would want an infinite
>> tolerance.
>>
>> So I'd propose to add your R_FINITE check, and make this change to the docs:
>>
>> " there is a relative tolerance of 1e-5, relative
>>    to the largest finite absolute value in the row"
>>
>> The copyright notice in the file says this function is from MASS/MASS.c
>> by W. N. Venables and B. D. Ripley, so I'd like to hear from at least
>> one of them before making a change.
>>
>> Duncan Murdoch
>>
>>> (Sorry to crosspost to both this list and R-help, but it was suggested
>>> that R-devel would be a more appropriate forum for this.)
>>>
>>> ______________________________________________
>>> R-devel at r-project.org mailing list
>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>
> 
> ______________________________________________
> R-devel at r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel



[Rd] max.col oddity
Duncan Murdoch