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

rownames .SD #3254

Merged
merged 2 commits into from
Jan 4, 2019
Merged

rownames .SD #3254

merged 2 commits into from
Jan 4, 2019

Conversation

mattdowle
Copy link
Member

@mattdowle mattdowle commented Jan 4, 2019

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
Copy link

codecov bot commented Jan 4, 2019

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
Copy link

codecov bot commented Jan 4, 2019

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