shift no longer works on matrix type under 1.14.3

[ethanbsmith]

shift used to work on matrix columns, but now generates and error. i dont see anything in the NEWS. is this intentional as its a breaking change?

works in 1.14.0

sessionInfo()
R version 4.1.2 (2021-11-01)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows 10 x64 (build 19044)

Matrix products: default

locale:
[1] LC_COLLATE=English_United States.1252  LC_CTYPE=English_United States.1252    LC_MONETARY=English_United States.1252 LC_NUMERIC=C                           LC_TIME=English_United States.1252    

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

loaded via a namespace (and not attached):
[1] compiler_4.1.2    tools_4.1.2       data.table_1.14.0 checkpoint_1.0.0 
> data.table::shift(matrix(1:10, ncol = 1))
 [1] NA  1  2  3  4  5  6  7  8  9

not working under 1.14.3

 sessionInfo()
R version 4.1.2 (2021-11-01)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows 10 x64 (build 19044)

Matrix products: default

locale:
[1] LC_COLLATE=English_United States.1252  LC_CTYPE=English_United States.1252    LC_MONETARY=English_United States.1252 LC_NUMERIC=C                           LC_TIME=English_United States.1252    

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

loaded via a namespace (and not attached):
[1] compiler_4.1.2    tools_4.1.2       data.table_1.14.3 checkpoint_1.0.0 
> data.table::shift(matrix(1:10, ncol = 1))
Error in data.table::shift(matrix(1:10, ncol = 1)) : 
  'as' must not be matrix or array

[ben-schwen]

Introduced in #5189. It's erroring because coerceAs cannot handle matrix/arrays.
Same error ofc also appears for nafill(matrix(c(NA, 1:9), ncol = 1), fill=0) which also happened to "work" before #4491.

According to docs shift/nafill never supported matrix but happened to work because matrices/array are R internally represented via one-dimensonial arrays with attributes.

@jangorecki are there reasons why coerceAs(x, as, copy) doesn't allow matrices/arrays for arguments x and as?

[jangorecki]

There reason is that there was no such requirement when it was implemented.
https://rdatatable.gitlab.io/data.table/library/data.table/html/shift.html does not mention matrix, but "A vector, list, data.frame or data.table" only, as Ben pointed out.

[MichaelChirico]

the fix in this case is very simple:

data.table::shift(c(matrix(1:10, ncol = 1)))

The earlier behavior on multi-column matrices is IMO quite undesirable, so I'm happy to break it in this case. Though we could improve the error message by catching it earlier.

[ethanbsmith]

for the record, im not asking for the old functionality to be restored. was more pointing out that it did break existing code, so probably warrants a section in the news if intentional (which it looks like it is)

…

________________________________ From: Michael Chirico ***@***.***> Sent: Saturday, December 18, 2021 11:35 PM To: Rdatatable/data.table ***@***.***> Cc: Ethan Smith ***@***.***>; Author ***@***.***> Subject: Re: [Rdatatable/data.table] `shift` no longer works on matrix type under 1.14.3 (Issue #5287) the fix in this case is very simple: data.table::shift(c(matrix(1:10, ncol = 1))) The earlier behavior on multi-column matrices is IMO quite undesirable, so I'm happy to break it in this case. Though we could improve the error message by catching it earlier. — Reply to this email directly, view it on GitHub<#5287 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AF2ACB3WRSFSLTBRJJ7TWLTURV4LXANCNFSM5J4UTY6Q>. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[jangorecki]

Though we could improve the error message by catching it earlier.

Was about to do it, but realized I will just copy error message from coerceAs into nafill and shift. IMO it's better to catch it there, in single a place.