Bug 17160

Summary: max(character(0), 1) returns incorrect value
Product: R Reporter: Alex Bertram <alex>
Component: LanguageAssignee: R-core <R-core>
Status: CLOSED FIXED    
Severity: normal CC: alex, maechler, suharto_anggono
Priority: P5    
Version: R 3.3.*   
Hardware: All   
OS: All   
Attachments: Fixes the issue
Test suite for min/max

Description Alex Bertram 2016-10-07 08:29:05 UTC
Both min() and max() incorrectly return NA if an empty character vector precedes non-empty numerical vectors:

> max(character(0), 1)
[1] NA
> max(1, character(0))
[1] "1"
> min(character(0), 1)
[1] NA
> min(1, character(0))
[1] "1"
> range(character(0), 1)
[1] "1" "1"
> range(1, character(0))
[1] "1" "1"
Comment 1 Alex Bertram 2016-10-07 10:28:15 UTC
Created attachment 2168 [details]
Fixes the issue
Comment 2 Alex Bertram 2016-10-07 10:43:09 UTC
Created attachment 2169 [details]
Test suite for min/max
Comment 3 Martin Maechler 2016-10-07 20:34:58 UTC
Thank you, Alex.   The patch is small and fine.

After finishing the tests, I'll commit to R-devel today ... with the plan to port to R-patched (planned to become R 3.3.2 by the end of the month).

Out of curiosity: how did you create your test suite?
Comment 4 Alex Bertram 2016-10-07 21:33:31 UTC
Terrific!

The test suite is generated by an R script here:
https://github.com/bedatadriven/renjin/blob/master/tests/src/testgen/gen-summary-tests.R

Mostly the idea is to generate a test suite that will ensure that Renjin's results match GNU R. Sometimes, when the test fails, I dig a bit deeper and question the results of GNU R. To arrive at the test suite I attached, I first generated the suite via R 3.3.1, identified the results I thought were not correct, patched R-trunk, and then regenerated the tests, verifying that only the problematic test cases were updated.

The test generation approach is very helpful for Renjin development, but might also be useful for GNU-- happy to contribute / collaborate on the growing test suite if there is interest.
Comment 5 Suharto Anggono 2016-10-08 13:36:23 UTC
(In reply to Alex Bertram from comment #1)
> Created attachment 2168 [details]
> Fixes the issue

There is a check
if(empty || stmp == NA_STRING || ...) .
The part is inside 'else' part of
if(empty) .
So, 'empty' is false there. It is not necessary to check 'empty' again.
Comment 6 Martin Maechler 2016-10-08 16:14:23 UTC
(In reply to Suharto Anggono from comment #5)
> (In reply to Alex Bertram from comment #1)
> > Created attachment 2168 [details]
> > Fixes the issue
> 
> There is a check
> if(empty || stmp == NA_STRING || ...) .
> The part is inside 'else' part of
> if(empty) .
> So, 'empty' is false there. It is not necessary to check 'empty' again.

Correct.  I also see another (very small) optimization slightly above.