Skip to content

rownames .SD - #3254

Merged
mattdowle merged 2 commits into
masterfrom
rownames_SD
Jan 4, 2019
Merged

mattdowle merged 2 commits into
masterfrom
rownames_SD

Conversation

@mattdowle

@mattdowle mattdowle commented Jan 4, 2019 •

Copy link
Copy Markdown
Member

Thanks to revdep checking of package rENA. Very rare case where rownames(.SD) are used; e.g. as.matrix(.SD) by group together with using .SDcols too and also using a column inside j={ } which is not in .SDcols. Problem arose in dev and not with CRAN version.

Separately, a one-line change is needed to rENA in one test, for reference from #3233.
I emailed rENA maintainer as follows :


Dear Cody,

Firstly, happy new year!

data.table v1.12.0 is about to go to CRAN and revdep checking shows it will break rENA.

This change affects rENA :
image
Please change line 105 of tests/testthat/test.ena.accumulate.data.file.R from :
testthat::expect_identical(x$adjacency.vectors, xtest[,grep("^adjacency", colnames(xtest)), with=F])
to
testthat::expect_equal(x$adjacency.vectors, xtest[,grep("^adjacency", colnames(xtest)), with=F], check.attributes=FALSE)

I've checked rENA passes v1.12.0 with this one line change.

Further, line 195 of ENAdata.R contains [,,.SDcols=metaAvail]. This now throws a warning that i and j are both missing so the other arguments (.SDcols= in this case) are being ignored. It's likely that line is not doing what you intended.

When data.table v1.12.0 goes to CRAN, rENA will start to fail. Could you update rENA on CRAN please. Either in advance or after data.table v1.12.0 is there is ok too.

(If you are on GitHub I would submit a pull request.)

Best, Matt


@mattdowle mattdowle added this to the 1.12.0 milestone Jan 4, 2019
@mattdowle mattdowle mentioned this pull request Jan 4, 2019
@codecov

codecov Bot commented Jan 4, 2019

Copy link
Copy Markdown

Codecov Report

Merging #3254 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3254      +/-   ##
==========================================
+ Coverage   94.65%   94.65%   +<.01%     
==========================================
  Files          65       65              
  Lines       12105    12106       +1     
==========================================
+ Hits        11458    11459       +1     
  Misses        647      647
Impacted Files Coverage Δ
src/dogroups.c 93.35% <100%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d965adb...f35a5cb. Read the comment docs.

1 similar comment
@codecov

codecov Bot commented Jan 4, 2019 •

Copy link
Copy Markdown

Codecov Report

Merging #3254 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3254      +/-   ##
==========================================
+ Coverage   94.65%   94.65%   +<.01%     
==========================================
  Files          65       65              
  Lines       12105    12106       +1     
==========================================
+ Hits        11458    11459       +1     
  Misses        647      647
Impacted Files Coverage Δ
src/dogroups.c 93.35% <100%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d965adb...f35a5cb. Read the comment docs.

@mattdowle
mattdowle merged commit d955ab7 into master Jan 4, 2019
@mattdowle
mattdowle deleted the rownames_SD branch January 4, 2019 03:31
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.

1 participant