Rdatatable / data.table

R's data.table package extends data.frame:
http://r-datatable.com
Mozilla Public License 2.0
3.56k stars 973 forks source link

revdep scoringutils (.optmean) errors after non-ascii by fix #6284

Closed tdhock closed 1 month ago

tdhock commented 1 month ago

https://github.com/Rdatatable/data.table/pull/4711 broke revdep scoringutils https://rcdata.nau.edu/genomic-ml/data.table-revdeps/analyze/2024-07-16/

> system(paste(c("diff -u", Rcheck.list), collapse=" "))
--- R_Under_development_unstable_2024-07-15_r86902/release_1.15.4.Rcheck/00check.log    2024-07-16 01:24:12.695554623 -0700
+++ R_Under_development_unstable_2024-07-15_r86902/master_1.15.99.9e2601e04a88610f28c798d047ebeb6103bd6fb2.Rcheck/00check.log   2024-07-16 01:27:23.045737401 -0700
@@ -54,13 +54,75 @@
 * checking data for ASCII and uncompressed saves ... OK
 * checking installed files from 'inst/doc' ... OK
 * checking files in 'vignettes' ... OK
-* checking examples ... OK
+* checking examples ... ERROR
+Running examples in 'scoringutils-Ex.R' failed
+The error most likely occurred in:
+
+> base::assign(".ptime", proc.time(), pos = "CheckExEnv")
+> ### Name: add_coverage
+> ### Title: Add coverage of central prediction intervals
+> ### Aliases: add_coverage
+> ### Keywords: scoring
+> 
+> ### ** Examples
+> 
+> library(magrittr) # pipe operator
+> ## Don't show: 
+>   data.table::setDTthreads(2) # restricts number of cores used on CRAN
+> ## End(Don't show)
+> score(example_quantile) %>%
++   add_coverage(by = c("model", "target_type")) %>%
++   summarise_scores(by = c("model", "target_type")) %>%
++   summarise_scores(fun = signif, digits = 2)
+The following messages were produced when checking inputs:
+1.  144 values for `prediction` are NA in the data provided and the corresponding rows were removed. This may indicate a problem if unexpected.
+Error in startsWith(names(expr)[3L], "na") : non-character object(s)
+Calls: %>% ... summarise_scores -> [ -> [.data.table -> lapply -> FUN -> startsWith
+Execution halted
 * checking for unstated dependencies in 'tests' ... OK
-* checking tests ... OK
+* checking tests ... ERROR
   Running 'testthat.R'
+Running the tests in 'tests/testthat.R' failed.
+Last 13 lines of output:
+  * plot_pit/plot-pit-quantile.svg
+  * plot_pit/plot-pit-sample.svg
+  * plot_predictions/many-quantiles-from-sample.svg
+  * plot_predictions/many-quantiles.svg
+  * plot_predictions/no-median.svg
+  * plot_predictions/point-forecasts.svg
+  * plot_quantile_coverage/plot-quantile-coverage.svg
+  * plot_ranges/plot-ranges-dispersion.svg
+  * plot_ranges/plot-ranges-interval.svg
+  * plot_score_table/plot-score-table.svg
+  * plot_wis/plot-wis-flip.svg
+  * plot_wis/plot-wis-no-relative.svg
+  * plot_wis/plot-wis.svg
+  Error: Test failures
+  Execution halted
 * checking for unstated dependencies in vignettes ... OK
 * checking package vignettes ... OK
-* checking re-building of vignette outputs ... OK
+* checking re-building of vignette outputs ... ERROR
+Error(s) in re-building vignettes:
+  ...
+--- re-building 'metric-details.Rmd' using rmarkdown
+--- finished re-building 'metric-details.Rmd'
+
+--- re-building 'scoring-forecasts-directly.Rmd' using rmarkdown
+--- finished re-building 'scoring-forecasts-directly.Rmd'
+
+--- re-building 'scoringutils.Rmd' using rmarkdown
+
+Quitting from lines 180-182 [unnamed-chunk-12] (scoringutils.Rmd)
+Error: processing vignette 'scoringutils.Rmd' failed with diagnostics:
+non-character object(s)
+--- failed re-building 'scoringutils.Rmd'
+
+SUMMARY: processing the following file failed:
+  'scoringutils.Rmd'
+
+Error: Vignette re-building failed.
+Execution halted
+
 * checking PDF version of manual ... OK
 * DONE
-Status: OK
+Status: 3 ERRORs
tdhock commented 1 month ago

@MichaelChirico can you please try to figure this one out?

MichaelChirico commented 1 month ago

Filed https://github.com/epiforecasts/scoringutils/pull/862

With that PR (and https://github.com/epiforecasts/scoringutils/pull/863 and https://github.com/epiforecasts/scoringutils/pull/864, which are unrelated to data.table per se), I get a clean R CMD check.

Note that it's nothing to do with #4711, so something seems wrong with the bisect logic there, otherwise we're seeing issues already fixed in the dev version of {scoringutils}

tdhock commented 1 month ago

thanks! from https://github.com/epiforecasts/scoringutils/pull/862 it looks like the fix was an updated expectation in a test. That seems to be inconsistent with the diff I reported above (ERROR in re-building vignettes and examples, in addition to tests). You are right that it could be explained by updates to scoringutils on github (revdep checker uses scoringutils from CRAN which was from Nov 2023).

tdhock commented 1 month ago

yes the two versions of scoringutils give different check results with data.table dev (there is no issue with revdep bisect)

(base) tdhock@maude-MacBookPro:~/R$ R CMD check scoringutils_1.2.2.tar.gz 
Loading required package: grDevices
* using log directory ‘/home/tdhock/R/scoringutils.Rcheck’
* using R version 4.4.1 (2024-06-14)
* using platform: x86_64-pc-linux-gnu
* R was compiled by
    gcc (GCC) 10.1.0
    GNU Fortran (GCC) 10.1.0
* running under: Ubuntu 18.04.6 LTS
* using session charset: UTF-8
* checking for file ‘scoringutils/DESCRIPTION’ ... OK
* this is package ‘scoringutils’ version ‘1.2.2’
* package encoding: UTF-8
* checking package namespace information ... OK
* checking package dependencies ... OK
* checking if this is a source package ... OK
* checking if there is a namespace ... OK
* checking for executable files ... OK
* checking for hidden files and directories ... OK
* checking for portable file names ... OK
* checking for sufficient/correct file permissions ... OK
* checking whether package ‘scoringutils’ can be installed ... OK
* checking installed package size ... OK
* checking package directory ... OK
* checking ‘build’ directory ... OK
* checking DESCRIPTION meta-information ... OK
* checking top-level files ... OK
* checking for left-over files ... OK
* checking index information ... OK
* checking package subdirectories ... OK
* checking code files for non-ASCII characters ... OK
* checking R files for syntax errors ... OK
* checking whether the package can be loaded ... OK
* checking whether the package can be loaded with stated dependencies ... OK
* checking whether the package can be unloaded cleanly ... OK
* checking whether the namespace can be loaded with stated dependencies ... OK
* checking whether the namespace can be unloaded cleanly ... OK
* checking whether startup messages can be suppressed ... OK
* checking dependencies in R code ... OK
* checking S3 generic/method consistency ... OK
* checking replacement functions ... OK
* checking foreign function calls ... OK
* checking R code for possible problems ... OK
* checking Rd files ... OK
* checking Rd metadata ... OK
* checking Rd cross-references ... OK
* checking for missing documentation entries ... OK
* checking for code/documentation mismatches ... OK
* checking Rd \usage sections ... OK
* checking Rd contents ... OK
* checking for unstated dependencies in examples ... OK
* checking contents of ‘data’ directory ... OK
* checking data for non-ASCII characters ... OK
* checking LazyData ... OK
* checking data for ASCII and uncompressed saves ... OK
* checking installed files from ‘inst/doc’ ... OK
* checking files in ‘vignettes’ ... OK
* checking examples ... ERROR
Running examples in ‘scoringutils-Ex.R’ failed
The error most likely occurred in:

> ### Name: add_coverage
> ### Title: Add coverage of central prediction intervals
> ### Aliases: add_coverage
> ### Keywords: scoring
> 
> ### ** Examples
> 
> library(magrittr) # pipe operator
> ## Don't show: 
>   data.table::setDTthreads(2) # restricts number of cores used on CRAN
> ## End(Don't show)
> score(example_quantile) %>%
+   add_coverage(by = c("model", "target_type")) %>%
+   summarise_scores(by = c("model", "target_type")) %>%
+   summarise_scores(fun = signif, digits = 2)
The following messages were produced when checking inputs:
1.  144 values for `prediction` are NA in the data provided and the corresponding rows were removed. This may indicate a problem if unexpected.
Error in startsWith(names(expr)[3L], "na") : non-character object(s)
Calls: %>% ... summarise_scores -> [ -> [.data.table -> lapply -> FUN -> startsWith
Execution halted
* checking for unstated dependencies in ‘tests’ ... OK
* checking tests ...
  Running ‘testthat.R’
 ERROR
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
  • plot_pit/plot-pit-quantile.svg
  • plot_pit/plot-pit-sample.svg
  • plot_predictions/many-quantiles-from-sample.svg
  • plot_predictions/many-quantiles.svg
  • plot_predictions/no-median.svg
  • plot_predictions/point-forecasts.svg
  • plot_quantile_coverage/plot-quantile-coverage.svg
  • plot_ranges/plot-ranges-dispersion.svg
  • plot_ranges/plot-ranges-interval.svg
  • plot_score_table/plot-score-table.svg
  • plot_wis/plot-wis-flip.svg
  • plot_wis/plot-wis-no-relative.svg
  • plot_wis/plot-wis.svg
  Error: Test failures
  Execution halted
* checking for unstated dependencies in vignettes ... OK
* checking package vignettes ... OK
* checking re-building of vignette outputs ... ERROR
Error(s) in re-building vignettes:
  ...
--- re-building ‘metric-details.Rmd’ using rmarkdown
--- finished re-building ‘metric-details.Rmd’

--- re-building ‘scoring-forecasts-directly.Rmd’ using rmarkdown
--- finished re-building ‘scoring-forecasts-directly.Rmd’

--- re-building ‘scoringutils.Rmd’ using rmarkdown

Quitting from lines 180-182 [unnamed-chunk-12] (scoringutils.Rmd)
Error: processing vignette 'scoringutils.Rmd' failed with diagnostics:
non-character object(s)
--- failed re-building ‘scoringutils.Rmd’

SUMMARY: processing the following file failed:
  ‘scoringutils.Rmd’

Error: Vignette re-building failed.
Execution halted

* checking PDF version of manual ... OK
* DONE

Status: 3 ERRORs
See
  ‘/home/tdhock/R/scoringutils.Rcheck/00check.log’
for details.

(base) tdhock@maude-MacBookPro:~/R$ R CMD check scoringutils_1.2.2.9000.tar.gz 
Loading required package: grDevices
* using log directory ‘/home/tdhock/R/scoringutils.Rcheck’
* using R version 4.4.1 (2024-06-14)
* using platform: x86_64-pc-linux-gnu
* R was compiled by
    gcc (GCC) 10.1.0
    GNU Fortran (GCC) 10.1.0
* running under: Ubuntu 18.04.6 LTS
* using session charset: UTF-8
* checking for file ‘scoringutils/DESCRIPTION’ ... OK
* this is package ‘scoringutils’ version ‘1.2.2.9000’
* package encoding: UTF-8
* checking package namespace information ... OK
* checking package dependencies ... OK
* checking if this is a source package ... OK
* checking if there is a namespace ... OK
* checking for executable files ... OK
* checking for hidden files and directories ... OK
* checking for portable file names ... OK
* checking for sufficient/correct file permissions ... OK
* checking whether package ‘scoringutils’ can be installed ... OK
* checking installed package size ... OK
* checking package directory ... OK
* checking ‘build’ directory ... OK
* checking DESCRIPTION meta-information ... OK
* checking top-level files ... OK
* checking for left-over files ... OK
* checking index information ... OK
* checking package subdirectories ... OK
* checking code files for non-ASCII characters ... OK
* checking R files for syntax errors ... OK
* checking whether the package can be loaded ... OK
* checking whether the package can be loaded with stated dependencies ... OK
* checking whether the package can be unloaded cleanly ... OK
* checking whether the namespace can be loaded with stated dependencies ... OK
* checking whether the namespace can be unloaded cleanly ... OK
* checking whether startup messages can be suppressed ... OK
* checking dependencies in R code ... OK
* checking S3 generic/method consistency ... OK
* checking replacement functions ... OK
* checking foreign function calls ... OK
* checking R code for possible problems ... OK
* checking Rd files ... OK
* checking Rd metadata ... OK
* checking Rd cross-references ... OK
* checking for missing documentation entries ... OK
* checking for code/documentation mismatches ... OK
* checking Rd \usage sections ... OK
* checking Rd contents ... OK
* checking for unstated dependencies in examples ... OK
* checking contents of ‘data’ directory ... OK
* checking data for non-ASCII characters ... OK
* checking LazyData ... OK
* checking data for ASCII and uncompressed saves ... OK
* checking installed files from ‘inst/doc’ ... OK
* checking files in ‘vignettes’ ... OK
* checking examples ... OK
* checking for unstated dependencies in ‘tests’ ... OK
* checking tests ...
  Running ‘testthat.R’
 ERROR
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
  • plot_avail_forecasts/plot-available-forecasts.svg
  • plot_correlation/plot-correlation.svg
  • plot_heatmap/plot-heatmap.svg
  • plot_interval_coverage/plot-interval-coverage.svg
  • plot_pairwise_comparison/plot-pairwise-comparison-pval.svg
  • plot_pairwise_comparison/plot-pairwise-comparison.svg
  • plot_pit/plot-pit-integer.svg
  • plot_pit/plot-pit-quantile.svg
  • plot_pit/plot-pit-sample.svg
  • plot_quantile_coverage/plot-quantile-coverage.svg
  • plot_wis/plot-wis-flip.svg
  • plot_wis/plot-wis-no-relative.svg
  • plot_wis/plot-wis.svg
  Error: Test failures
  Execution halted
* checking for unstated dependencies in vignettes ... OK
* checking package vignettes ... OK
* checking re-building of vignette outputs ... OK
* checking PDF version of manual ... WARNING
LaTeX errors when creating PDF version.
This typically indicates Rd problems.
LaTeX errors found:
! Package inputenc Error: Unicode char − (U+2212)
(inputenc)                not set up for use with LaTeX.

See the inputenc package documentation for explanation.
Type  H <return>  for immediate help.
* checking PDF version of manual without index ... ERROR
* DONE

Status: 2 ERRORs, 1 WARNING
See
  ‘/home/tdhock/R/scoringutils.Rcheck/00check.log’
for details.

(base) tdhock@maude-MacBookPro:~/R$ cat scoringutils.Rcheck/tests/testthat.Rout.fail 

R version 4.4.1 (2024-06-14) -- "Race for Your Life"
Copyright (C) 2024 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

> library(testthat)
> library(scoringutils)
Note: scoringutils is currently undergoing major development changes (with an update planned for the first quarter of 2024). We would very much appreciate your opinions and feedback on what should be included in this major update: https://github.com/epiforecasts/scoringutils/discussions/333
> 
> test_check("scoringutils")
i Some rows containing NA values may be removed. This is fine if not unexpected.
i Some rows containing NA values may be removed. This is fine if not unexpected.
Forecast unit:
location, target_end_date, target_type, location_name, forecast_date, model,
and horizon
i Some rows containing NA values may be removed. This is fine if not unexpected.
i Some rows containing NA values may be removed. This is fine if not unexpected.
i Some rows containing NA values may be removed. This is fine if not unexpected.
i Some rows containing NA values may be removed. This is fine if not unexpected.
i Some rows containing NA values may be removed. This is fine if not unexpected.
i Some rows containing NA values may be removed. This is fine if not unexpected.
[ FAIL 1 | WARN 1 | SKIP 16 | PASS 487 ]

══ Skipped tests (16) ══════════════════════════════════════════════════════════
• On CRAN (16): 'test-plot_avail_forecasts.R:12:3',
  'test-plot_correlation.R:10:3', 'test-plot_heatmap.R:9:3',
  'test-plot_interval_coverage.R:8:3', 'test-plot_pairwise_comparison.R:9:3',
  'test-plot_pairwise_comparison.R:17:3', 'test-plot_pit.R:8:3',
  'test-plot_pit.R:20:3', 'test-plot_pit.R:30:3',
  'test-plot_quantile_coverage.R:9:3', 'test-plot_wis.R:24:3',
  'test-plot_wis.R:35:3', 'test-plot_wis.R:47:3', 'test-print.R:11:5',
  'test-utils_data_handling.R:157:3', 'test-utils_data_handling.R:211:3'

══ Failed tests ════════════════════════════════════════════════════════════════
── Failure ('test-utils_data_handling.R:32:3'): quantile_to_interval_dataframe() works ──
`quantile_to_interval(quantile, keep_quantile_col = FALSE, format = "wide")` did not throw the expected message.

[ FAIL 1 | WARN 1 | SKIP 16 | PASS 487 ]
Deleting unused snapshots:
• plot_avail_forecasts/plot-available-forecasts.svg
• plot_correlation/plot-correlation.svg
• plot_heatmap/plot-heatmap.svg
• plot_interval_coverage/plot-interval-coverage.svg
• plot_pairwise_comparison/plot-pairwise-comparison-pval.svg
• plot_pairwise_comparison/plot-pairwise-comparison.svg
• plot_pit/plot-pit-integer.svg
• plot_pit/plot-pit-quantile.svg
• plot_pit/plot-pit-sample.svg
• plot_quantile_coverage/plot-quantile-coverage.svg
• plot_wis/plot-wis-flip.svg
• plot_wis/plot-wis-no-relative.svg
• plot_wis/plot-wis.svg
Error: Test failures
Execution halted
(base) tdhock@maude-MacBookPro:~/R$ 
tdhock commented 1 month ago

in scoringutils CRAN version, the error occurs here

  scores <- scores[, lapply(.SD, base::mean, ...),
    by = c(unique(c(forecast_unit, by))),
    .SDcols = colnames(scores) %like% cols_to_summarise
  ]

Traceback is

> traceback()
6: startsWith(names(expr)[3L], "na")
5: FUN(X[[i]], ...)
4: lapply(jsub[w], .optmean)
3: `[.data.table`(scores, , lapply(.SD, base::mean, ...), by = c(unique(c(forecast_unit, 
       by))), .SDcols = colnames(scores) %like% cols_to_summarise) at summarise_scores.R#146
2: scores[, lapply(.SD, base::mean, ...), by = c(unique(c(forecast_unit, 
       by))), .SDcols = colnames(scores) %like% cols_to_summarise] at summarise_scores.R#146
1: summarise_scores(scores)

The startsWith call comes from this function in data.table.R

.optmean = function(expr) {   # called by optimization of j inside [.data.table only. Outside for a small speed advantage.
  if (length(expr)==2L)  # no parameters passed to mean, so defaults of trim=0 and na.rm=FALSE
    return(call(".External",quote(Cfastmean),expr[[2L]], FALSE))
    # return(call(".Internal",expr))  # slightly faster than .External, but R now blocks .Internal in coerce.c from apx Sep 2012
  if (length(expr)==3L && startsWith(names(expr)[3L], "na"))   # one parameter passed to mean()
    return(call(".External",quote(Cfastmean),expr[[2L]], expr[[3L]]))  # faster than .Call
  assign("nomeanopt",TRUE,parent.frame())
  expr  # e.g. trim is not optimized, just na.rm
}
MichaelChirico commented 1 month ago

Oh, actually it's ~#6284~ #6232 , not #4711 (one commit later) to blame. See #6291.

Thanks for the further investigation, even though revdep dev version no longer errors, the original code still helped us to discover a bug.

tdhock commented 1 month ago

Oh, actually it's https://github.com/Rdatatable/data.table/issues/6284, not https://github.com/Rdatatable/data.table/pull/4711

Is that a typo? I still don't understand why #4711 came out of git bisect.

MichaelChirico commented 1 month ago

Is that a typo?

Yes: #6232 (3ca83738d70d5597d9e168077f3768e32569c790) hit master one before #4711 (40758c9250b66bb5156418c0c47db08c98d4adce).

tdhock commented 1 month ago

ok, #6232 makes sense as the culprit since our version of startsWith was removed in that PR, and startsWith caused the error in the revdep. so maybe there is an issue with revdep bisect after all? I don't really understand why git bisect would point to the PR after the culprit, I will file an issue.

tdhock commented 1 month ago

could it have something to do with changing %iscall% in #4711 ? https://github.com/Rdatatable/data.table/commit/40758c9250b66bb5156418c0c47db08c98d4adce#diff-60bef5af43a1bd541299ce9bceeb226afb23deaa5e637b145c85e05ffd552b0cR152

MichaelChirico commented 1 month ago

I suppose it's possible downstream code wound up in a different branch due to the change, where before #4711 startsWith() doesn't get called.

MichaelChirico commented 1 month ago

Informed downstream that we'll release in August & to expect a required CRAN update (their dev version is already fixed); closing here.