Repository navigation
revdep failure from dcast error if fun.aggregate returns length>1 #6032
Description
Activity
looks like the issue may be with the revdeps. At least in mcchart, fun.aggregate=identity, which is indeed invalid, since identity usually returns vector with length>1, whereas fun.aggregate is supposed to return vector of length=1, as documented in the error message.
Browse[2]> fun.call list(Sepal.Width = (function (x) { x })(Sepal.Width)) Browse[2]> list.of.columns $Sepal.Width [1] 3.5 3.5 3.8 3.7 3.3 3.4 3.8 3.8
same with maditr, fun.aggregate=identity is being used.
Browse[2]> dat Species variable Sepal Petal <fctr> <fctr> <num> <num> 1: setosa Length 5.1 1.4 2: setosa Length 4.9 1.4 3: setosa Length 4.7 1.3 4: setosa Length 4.6 1.5 5: setosa Length 5.0 1.4 --- 296: virginica Width 3.0 2.3 297: virginica Width 2.5 1.9 298: virginica Width 3.0 2.0 299: virginica Width 3.4 2.3 300: virginica Width 3.0 1.8 Browse[2]> varnames [1] "Species" "variable" Browse[2]> list.of.columns $Sepal [1] 5.1 4.9 4.7 4.6 5.0 5.4 4.6 5.0 4.4 4.9 5.4 4.8 4.8 4.3 5.8 5.7 5.4 5.1 5.7 [20] 5.1 5.4 5.1 4.6 5.1 4.8 5.0 5.0 5.2 5.2 4.7 4.8 5.4 5.2 5.5 4.9 5.0 5.5 4.9 [39] 4.4 5.1 5.0 4.5 4.4 5.0 5.1 4.8 5.1 4.6 5.3 5.0 $Petal [1] 1.4 1.4 1.3 1.5 1.4 1.7 1.4 1.5 1.4 1.5 1.5 1.6 1.4 1.1 1.2 1.5 1.3 1.4 1.7 [20] 1.5 1.7 1.5 1.0 1.7 1.9 1.6 1.6 1.5 1.4 1.6 1.6 1.5 1.5 1.4 1.5 1.2 1.3 1.4 [39] 1.3 1.5 1.3 1.3 1.3 1.6 1.9 1.4 1.6 1.4 1.5 1.4 Browse[2]> fun.call list(Sepal = (function (x) x)(Sepal), Petal = (function (x) x)(Petal))
this is documented in ?dcast details, so I will file issues with revdeps.
The aggregating function should take a vector as input and return a single value (or a list of length one) as output. In cases where ‘value.var’ is a list, the function should be able to handle a list input and provide a single value or list of length one as output.We have a new revdep WES, Published: 2024-07-12, I will file an issue.
* checking examples ... ERROR Running examples in 'WES-Ex.R' failed The error most likely occurred in: > base::assign(".ptime", proc.time(), pos = "CheckExEnv") > ### Name: apply_delta_delta_ct > ### Title: Apply the delta delta Ct calculation to a data.frame > ### Aliases: apply_delta_delta_ct > > ### ** Examples > > > pae <- apply_amplification_efficiency(template_WES_standard_curve) > > ddct_standard <- apply_delta_delta_ct(df = template_WES_data, + target_names = c('target_1', 'target_2', 'target_3'), + reference_names = rep('target_0', 3)) Error in maybe_err(eval(fun.call)) : Aggregating function(s) should take vector inputs and return a single value (length=1). However, function(s) returns length!=1. This value will have to be used to fill any missing combinations, and therefore must be length=1. Either override by setting the 'fill' argument explicitly or modify your function to handle this case appropriately. Calls: apply_delta_delta_ct ... [.data.table -> maybe_err -> stopf -> raise_condition -> signal Execution halted- changed the title
[-]revdeps maditr, mschart example failures[/-][+]revdep failure from dcast error if fun.aggregate returns length>1[/+]on Jul 16, 2024 @tdhock given that this broke several downstreams, can we be sure to mention this under BREAKING CHANGES? (or possibly better, adjust our code so that it doesn't break the downstreams, e.g. a branch with
if (identical(fun.aggregate, identity)) ...that starts a deprecation cycle)Here are two more CRAN downstreams that were not caught by revdeps, because downstream usage is not in a tested region of code:
https://github.com/search?q=org%3Acran%20%2Ffun%5B.%5Daggregate%5Cs*%3D%5Cs*identity%2F&type=code
And several more such downstreams on GitHub:
https://github.com/search?q=lang%3AR+%2Ffun%5B.%5Daggregate%5Cs*%3D%5Cs*identity%2F&type=code
Thanks for your investigation Michael, that is useful.
There are possibly uses of other functions than identity, which return length>1, and would be additional breakages.
Not sure if this warrants a "deprecation" cycle since it was already documented, and this "breakage" brings the implementation up to date with the docs. But I guess we could.
Which option do you think would be more work, or less work, for us and downstreams?There are possibly uses of other functions than identity, which return length>1, and would be additional breakages.
Definitely possible -- have we seen any though? I would be happy special-casing this
fun.aggregate=identitygiven how common it is empirically:if (identical(fun.aggregate, identity)) { warningf("This is no longer supported, here's how to do what you wanted to... This will become an error on next release") return(old_behavior()) }
It looks (?) like the old behavior is equivalent to
fun.aggregate = function(x) x[1L], is that right?DT=data.table(a = rep(1:3, 2), b=rep(1:2, each=3), c=1:6) DT = rbind(DT, DT) dcast(DT, a ~ b, fun.aggregate=identity, value.var='c', fill=0) # Key: <a> # a 1 2 # <int> <int> <int> # 1: 1 1 4 # 2: 2 2 5 # 3: 3 3 6
so that branch could be
fun.aggregate <- function(x) x[1L]instead ofreturn(old_behavior())Hi Michael
I have not seen any problematic functions other than identity.
However I don't think we need to special case that.
In #6329 I propose to change the error to a warning, and that should be enough to alert revdeps, while avoiding any breaking changes.
Also I believe the old/undefined behavior was to use the last value (not first),> library(data.table) data.table 1.15.4 using 3 threads (see ?getDTthreads). Latest news: r-datatable.com > dcast(data.table(y="a",x=1:2), y ~ ., identity, fill=NA) Using 'x' as value column. Use 'value.var' to override Key: <y> y . <char> <int> 1: a 2
revdep checker confirms this is no longer an issue
after merging #5549 we get the following two new revdep check errors, not sure why, still need to investigate.
and