Repository navigation
revdep vardpoor example failures after melt change #6071
Description
Activity
using data.table from CRAN, and adding print statements to vardpoor, I get the output below
vrdnnl> vardannual(Y = "employed", H = "strata", vrdnnl+ PSU = "PSU", w_final = "rb050", vrdnnl+ ID_level1 = "db030", ID_level2 = "id_lv2", vrdnnl+ Dom = NULL, Z = NULL, years = "year", vrdnnl+ subperiods = "half", dataset = dataset1, vrdnnl+ percentratio = 100, confidence = 0.95, vrdnnl+ method = "cros") [1] "X" num [1:2] 242 1263 [1] "A_matrix" num [1:2, 1:2] 1 0.375 0.375 1 [1] "X" num [1:2] 1272 1542 [1] "A_matrix" num [1:2, 1:2] 1 -1 -1 1
Using data.table from github master I get output below
vrdnnl> vardannual(Y = "employed", H = "strata", vrdnnl+ PSU = "PSU", w_final = "rb050", vrdnnl+ ID_level1 = "db030", ID_level2 = "id_lv2", vrdnnl+ Dom = NULL, Z = NULL, years = "year", vrdnnl+ subperiods = "half", dataset = dataset1, vrdnnl+ percentratio = 100, confidence = 0.95, vrdnnl+ method = "cros") [1] "X" num(0) [1] "A_matrix" num [1:2, 1:2] 1 NA NA 1 Erreur dans t(X) %*% A_matrix : arguments inadéquats Appels : example ... eval -> eval -> vardannual -> lapply -> FUN -> data.table Exécution arrêtée
vardannual calls vardcros which calls melt with measure via the code below
if (!is.null(namesZ)) varsYZ <- list(namesY, namesZ) str(list(varsYZ=varsYZ, DTagg=DTagg)) DTagg <- melt(DTagg, id = c(namesperc, gnamesDom), measure = varsYZ, variable.factor = FALSE)
output below using data.table master:
List of 2 $ varsYZ:List of 1 ..$ : chr "employed" $ DTagg :Classes ‘data.table’ and 'data.frame': 4 obs. of 3 variables: ..$ pers : chr [1:4] "2010__1" "2010__2" "2011__1" "2011__2" ..$ percoun : chr [1:4] "1" "1" "1" "1" ..$ employed: num [1:4] 2742 3479 2259 2622 ..- attr(*, "sorted")= chr [1:2] "pers" "percoun" ..- attr(*, ".internal.selfref")=<externalptr>
and same for data.table release, which suggests varsYZ (list of length 1) should be changed to character vector.
the fix is to add the following line in the revdep code, I will file a PR.
if (length(varsYZ)==1) varsYZ <- unlist(varsYZ)
From report you filled downstream it doesn't sound it was a bug in revdep code. Therefore why not to add, at least, one extra release cycle for transition, not only CRAN packages but all the others? Currently used approach was never used in data.table, Matt was pulling out breaking changes, and sometimes even deciding to not proceed with them at all. Giving extra release cycle of time, and a notice in NEWS file sounds as softer way to approach this change.
Reacted by Toby Dylan Hocking and Michael Chiricogood idea
I think in this case, because downstream is relying on undocumented behavior, we can be a bit more aggressive than usual: I would move to a warning in the next release, then error, then remove code supporting the old way.
warning means that a package check will start to fail already from next release, and that sounds discouraging for new pkgs to import DT
warning means that a package check will start to fail already from next release, and that sounds discouraging for new pkgs to import DT
warning in example is not registered by
R CMD check(just confirmed)@tdhock I filed a suggested improvement to your downstream PR. However, IMO it makes their code superficially worse by introducing inconsistency across the
length==1/length>1cases -- is there a strong reason not to support that style (e.g. doing so would require some contradictory/collided behavior & we prefer the new approach)?If we plan to go ahead with breaking downstream, please make sure to add a relevant NEWS item explaining the change of behavior, and inform downstream of the imminent release; otherwise, please help to file a PR for the continued support of
measure=list(<single vector>).Right, the breaking change was made for increased consistency in data.table, see #5209
This is another example of bringing the implementation up to date with the docs, so not sure if a deprecation cycle is warranted. It definitely would be easier for us not to do one.
Since this is a weird corner case, and there is only one known revdep, I tend to err on the side of just making & explaining the breaking change. What do you think?
More generally, we should probably write a policy for when breaking changes are warranted (and when deprecation cycles are necessary). I believe the case of bringing the implementation up to date with the docs should be one example when a breaking change is warranted.I do agree it's not ideal to bend over backwards for one revdep, however
- not all revdeps that are broken will be revealed by our testing -- many packages simply have poor test coverage!
- user space (scripts, not packages) is even bigger and almost completely unknown/untested
So we tend to err on the side of caution wherever possible -- that's the "strong preference" for back-compatibility in the GOVERNANCE doc.
Adding consistency is good, but unless there's a new code path that prevents us from maintaining more back-compatibility in the short term, we should endeavor not to break downstreams without any deprecation cycle.
In this case, too, I worry from the example we did find that we are reducing consistency -- downstream goes from doing
if (is.null(B)) list(A) else list(A, B)(parallel use oflist()across branches) toif (is.null(B)) A else list(A, B); the former is more consistent to my reading. Is there an even nicer approach they should be using instead?More generally, we should probably write a policy for when breaking changes are warranted (and when deprecation cycles are necessary).
I agree it would be useful to have this written out.
Reacted by Toby Dylan Hockingrevdep checks confirm this is no longer an issue for vardpoor
I agree it would be useful to have this written out.
Here is a first attempt:
https://github.com/Rdatatable/data.table/wiki/Release-management-and-revdep-checks#when-is-a-breaking-change-warrantedThis issue is happening again (as expected) since #7266
A PR that fixes the revdep was filed over 1 year ago CSBLatvia/vardpoor#26
Reacted by Jan GoreckiSince the fix was filed over a year ago, can we close this for the 1.18.0 milestone?
Reacted by Jan Gorecki
after this commit 8fa8c8f
we get the following, which I will investigate.