Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow NULL dimnames to propagate through new_table() #1889

Merged
merged 3 commits into from
Nov 2, 2023

Conversation

DavisVaughan
Copy link
Member

R-devel failures:

══ Failed tests ════════════════════════════════════════════════════════════════
     ── Failure ('test-type-table.R:21:3'): can find a common type among tables with identical dimensions ──
     `actual` (vec_ptype2(tab1, tab1)) not identical to `expected` (zap_dimnames(new_table())).
    
     `dimnames(actual)` is a list
     `dimnames(expected)` is absent
     Backtrace:1. └─vctrs:::expect_identical(vec_ptype2(tab1, tab1), zap_dimnames(new_table())) at test-type-table.R:21:2
     2. └─vctrs:::expect_waldo_equal("identical", act, exp, info, ...) at tests/testthat/helper-vctrs.R:43:2
     ── Failure ('test-type-table.R:29:3'): size is not considered in the ptype ─────
     `actual` (vec_ptype2(x, y)) not identical to `expected` (zap_dimnames(new_table())).
    
     `dimnames(actual)` is a list
     `dimnames(expected)` is absent
     Backtrace:1. └─vctrs:::expect_identical(vec_ptype2(x, y), zap_dimnames(new_table())) at test-type-table.R:29:2
     2. └─vctrs:::expect_waldo_equal("identical", act, exp, info, ...) at tests/testthat/helper-vctrs.R:43:2
     ── Failure ('test-type-table.R:81:3'): `table` delegates coercion ──────────────
     `actual` (vec_ptype2(new_table(1), new_table(FALSE))) not identical to `expected` (zap_dimnames(new_table(double()))).
    
     `dimnames(actual)` is a list
     `dimnames(expected)` is absent
     Backtrace:1. └─vctrs:::expect_identical(...) at test-type-table.R:81:2
     2. └─vctrs:::expect_waldo_equal("identical", act, exp, info, ...) at tests/testthat/helper-vctrs.R:43:2
    
     [ FAIL 3 | WARN 0 | SKIP 308 | PASS 5289 ]
     Error: Test failures
     Execution halted
Flavor: [r-devel-linux-x86_64-fedora-clang](https://www.r-project.org/nosvn/R.check/r-devel-linux-x86_64-fedora-clang/vctrs-00check.html)

I have tracked this down to this undocumented R-devel change:

wch/r-source@2653cc6#diff-dbc5252913c71f32fa7295e497486c4c2b07ff43aa51b25c33e4dbdd397d3714R1249-R1253

// currently it is documented that `dim<-` removes dimnames() .. but ..
    SEXP odim = getAttrib0(vec, R_DimSymbol); // keep dimnames(.) if dim() entries are unchanged
    if((LENGTH(odim) != ndim) || memcmp((void *)INTEGER(odim),
					(void *)INTEGER(val), ndim * sizeof(int)))
	removeAttrib(vec, R_DimNamesSymbol);

This applies to dim<- and Rf_setAttrib(*, R_DimSymbol, *), which is what causes us issues.


Previously, in #1230 we were fairly aggressive about making the dimnames attribute returned from new_table() type stable, i.e. always a list() with either NULL or character vector elements.

This causes us some grief in vec_ptype2.table.table() because of the way Rf_setAttrib(*, R_DimSymbol, *) changed:

In R release:

  • If vec_ptype2(tbl[, 2], tbl[, 2]) are combined, there is no dimnames attribute
  • If vec_ptype2(tbl[, 1], tbl[, 2]) are combined, there is no dimnames attribute

In R-devel:

  • If vec_ptype2(tbl[, 2], tbl[, 2]) are combined, we get list(NULL, NULL) dimnames (from our attempt to type stabilize them in new_table() + R-devel leaving them alone since the dimensions are the same)
  • If vec_ptype2(tbl[, 1], tbl[, 2]) are combined, there is no dimnames attribute

I think the easiest thing to do is to just say that new_table() propagates NULL dimnames untouched, resulting in no dimnames attribute. I don't think vec_ptype2() should ever return any "names" related information, so this ensures we consistently don't add a dimnames attribute in R-devel and R-release.

@DavisVaughan
Copy link
Member Author

R core ended up reverting this (thanks to Henrik on the mailing list) but I'm still going to merge because it is a candidate for further discussion and I think this PR improves things a little anyways wch/r-source@13f79d3

@DavisVaughan DavisVaughan merged commit 19f1635 into r-lib:main Nov 2, 2023
12 checks passed
@DavisVaughan DavisVaughan deleted the fix/r-devel-dimnames branch November 2, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants