Bug 17160 - max(character(0), 1) returns incorrect value
Summary: max(character(0), 1) returns incorrect value
Status: CLOSED FIXED
Alias: None
Product: R
Classification: Unclassified
Component: Language (show other bugs)
Version: R 3.3.*
Hardware: All All
: P5 normal
Assignee: R-core
URL:
Depends on:
Blocks:
 
Reported: 2016-10-07 08:29 UTC by Alex Bertram
Modified: 2016-10-08 16:14 UTC (History)
3 users (show)

See Also:


Attachments
Fixes the issue (911 bytes, patch)
2016-10-07 10:28 UTC, Alex Bertram
Details | Diff
Test suite for min/max (409.00 KB, text/plain)
2016-10-07 10:43 UTC, Alex Bertram
Details

Note You need to log in before you can comment on or make changes to this bug.
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.