Bug 17300 - Patch: avoid deep recursion in methods:::cbind and methods:::rbind
Summary: Patch: avoid deep recursion in methods:::cbind and methods:::rbind
Status: CLOSED FIXED
Alias: None
Product: R
Classification: Unclassified
Component: Wishlist (show other bugs)
Version: R-devel (trunk)
Hardware: All All
: P5 enhancement
Assignee: R-core
URL: https://stat.ethz.ch/pipermail/r-deve...
Depends on:
Blocks:
 
Reported: 2017-06-30 12:14 UTC by Suharto Anggono
Modified: 2017-08-06 12:21 UTC (History)
2 users (show)

See Also:


Attachments
cbind.R and rbind.R in methods (6.36 KB, patch)
2017-06-30 12:14 UTC, Suharto Anggono
Details | Diff
cbind.R and rbind.R in package methods (6.36 KB, patch)
2017-06-30 12:31 UTC, Suharto Anggono
Details | Diff
cbind.R and rbind.R in package methods (6.37 KB, patch)
2017-06-30 12:46 UTC, Suharto Anggono
Details | Diff
Package methods: rbind.R: missed change (277 bytes, patch)
2017-07-27 16:02 UTC, Suharto Anggono
Details | Diff
Package methods: cbind.R, rbind.R (1.51 KB, patch)
2017-08-06 09:06 UTC, Suharto Anggono
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Suharto Anggono 2017-06-30 12:14:02 UTC
Created attachment 2269 [details]
cbind.R and rbind.R in methods

From https://stat.ethz.ch/pipermail/r-devel/2016-May/072682.html :

rbind() and cbind() are able to deal with S4 "matrix-like" objects, via the
hidden methods:::rbind /  methods:::cbind functions where these recursively build on appropriate S4 methods for rbind2() / cbind2().

A simple MRE (minimal reproducible example) for the problem seen with Matrix:

  S <- sparseMatrix(i=1:4, j=9:6, x=pi*1:4)
  n <- 410 # it succeeds for 407 -- sometimes even with 408
  X <- replicate(n, S, simplify = FALSE)
  Xb <- do.call("rbind", X)
  ## -> Error in as(x, "CsparseMatrix") : node stack overflow


---------------------------
The source of deep recursion is that the code of methods:::cbind uses do.call(cbind, *) for 3 or more arguments. The code of methods:::rbind is similar. This patch avoid it. Effectively, this calls 'cbind2' iteratively.
Comment 1 Suharto Anggono 2017-06-30 12:31:18 UTC
Created attachment 2270 [details]
cbind.R and rbind.R in package methods
Comment 2 Suharto Anggono 2017-06-30 12:46:32 UTC
Created attachment 2271 [details]
cbind.R and rbind.R in package methods
Comment 3 Michael Lawrence 2017-07-03 02:41:40 UTC
Thanks. I will work on merging this.
Comment 4 Suharto Anggono 2017-07-27 16:02:34 UTC
Created attachment 2286 [details]
Package methods: rbind.R: missed change

Sorry, attachment 2271 [details] has this change to cbind.R, but not to rbind.R.
'iV' is not used afterwards.
Comment 5 Suharto Anggono 2017-08-04 16:41:25 UTC
Could the patch in attachment 2286 [details] be applied?
Comment 6 Michael Lawrence 2017-08-04 16:56:34 UTC
Applied, thanks for the contribution.
Comment 7 Suharto Anggono 2017-08-06 09:06:32 UTC
Created attachment 2287 [details]
Package methods: cbind.R, rbind.R
Comment 8 Suharto Anggono 2017-08-06 12:21:41 UTC
attachment 2287 [details]: fix 'N2' in 'rbind', fix commented code, cleaner 'ism2'

Embarassingly for me, attachment 2271 [details] has another change that is applied to cbind.R and not to rbind.R. Assignment to 'N2' should use Nms(na), not Nms(2). An example that gives wrong row names because of it, modified from the example in reg-tests-1d.R:
library(methods)
myM <- setClass("myMatrix", contains="matrix")
rbind(1:2, c=2, myM(4:1,2), "a+"=10)

For 'rbind', result like in R 3.4.1 can be obtained by uncommenting the following.
    ## if(deparse.level == 0)
    ##     if(i == 1L) return(r) else next

With changed 'ism2', it behaves like the comment "and since it's a 'matrix' now, cbind() below may not name it" and like in R 3.4.1. With the original 'ism2', when 'fix.na' is TRUE, the last name is set twice, inside and outside the 'for' loop.