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

Let print.data.table return invisible(x) #2807

Merged
merged 2 commits into from Apr 29, 2018

Conversation

@heavywatal
Copy link
Contributor

heavywatal commented Apr 29, 2018

print.data.table should invisibly return its first argument instead of NULL. It is compatible with the standard print.data.frame and tibble's print.tbl_df, and supports pipe-style coding:

head_iris = iris %>%
  as.data.table() %>%
  print() %>%
  head(3L) %>%
  print()

I would be happy to add a bullet on the NEWS if this PR is accepted. Maybe NOTES section? Thanks.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 29, 2018

Codecov Report

Merging #2807 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2807   +/-   ##
=======================================
  Coverage   93.49%   93.49%           
=======================================
  Files          61       61           
  Lines       12368    12368           
=======================================
  Hits        11563    11563           
  Misses        805      805
Impacted Files Coverage Δ
R/print.data.table.R 98.13% <100%> (ø) ⬆️

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 6aa0fc1...64f2abf. Read the comment docs.

Remove extra parentheses in my previous PR #2804
@MichaelChirico

This comment has been minimized.

Copy link
Member

MichaelChirico commented Apr 29, 2018

if i'm not mistaken (i may well be) the thinking behind not doing this in the first place is that always returning (e.g.) a 7GB data.table will be costly.

@heavywatal

This comment has been minimized.

Copy link
Contributor Author

heavywatal commented Apr 29, 2018

Good point. But an object seems not to be copied until it is modified (in a normal way). Maybe only a pointer/reference to the original object is returned by return(invisible(x)).

library(data.table)                                         
x = as.data.table(iris)                                     
tracemem(x)                                                 
#> [1] "<0x7f9d00dcc400>"
y = print(x, topn=1L)                                       
#>      Sepal.Length Sepal.Width Petal.Length Petal.Width   Species
#>   1:          5.1         3.5          1.4         0.2    setosa
#>  ---                                                            
#> 150:          5.9         3.0          5.1         1.8 virginica
tracemem(y)                                                 
#> [1] "<0x7f9d00dcc400>"
y[, SPECIES := toupper(Species)]   # in-place; affects x too
print(x, topn=1L)                                           
#>      Sepal.Length Sepal.Width Petal.Length Petal.Width   Species   SPECIES
#>   1:          5.1         3.5          1.4         0.2    setosa    SETOSA
#>  ---                                                                      
#> 150:          5.9         3.0          5.1         1.8 virginica VIRGINICA
y$species = tolower(y$SPECIES)     # copied for the first time!
#> tracemem[0x7f9d00dcc400 -> 0x7f9d02f78148]: eval eval withVisible withCallingHandlers doTryCatch tryCatchOne tryCatchList tryCatch try handle timing_fn evaluate_call <Anonymous> evaluate in_dir block_exec call_block process_group.block process_group withCallingHandlers process_file <Anonymous> <Anonymous> <Anonymous> <Anonymous> do.call saveRDS withCallingHandlers 
#> tracemem[0x7f9d02f78148 -> 0x7f9d02f780d8]: copy $<-.data.table $<- eval eval withVisible withCallingHandlers doTryCatch tryCatchOne tryCatchList tryCatch try handle timing_fn evaluate_call <Anonymous> evaluate in_dir block_exec call_block process_group.block process_group withCallingHandlers process_file <Anonymous> <Anonymous> <Anonymous> <Anonymous> do.call saveRDS withCallingHandlers
@mattdowle

This comment has been minimized.

Copy link
Member

mattdowle commented Apr 29, 2018

There's no speed or memory penalty to returning invisible(x). Isn't it related to auto-printing and :=? I just looked up our FAQ for the first time in a long time (where have all the FAQ numbers gone!!) and there's one there about auto-printing and invisible() [ that was a nightmare at the time]. But that FAQ doesn't help really.

Well, it passes tests, which includes auto-printing (/tests/autoprint.R) so on that basis I can't think why not.

Does it pass test.data.table(with.other.packages=TRUE) ?
Does it pass (stated dependency) R 3.1.0 ?

@MichaelChirico

This comment has been minimized.

Copy link
Member

MichaelChirico commented Apr 29, 2018

^ as matt was writing i was running a quick benchmark to convince myself there's no speed penalty; confirmed on a 15GB data.table there's no speed difference.

Can't see why it would fail on 3.1.0...

@jangorecki

This comment has been minimized.

Copy link
Member

jangorecki commented Apr 29, 2018

I like this change, as there is no speed penalty we should definitely go for consistency with data.frame. R 3.1.0 build runs for branches in our repo (as gitlab only mirrors our repo, not forks). Once merged I will take a look at master if it pass.
Running: https://gitlab.com/Rdatatable/data.table/-/jobs/65551303

@mattdowle mattdowle merged commit 26ed59f into Rdatatable:master Apr 29, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mattdowle mattdowle added this to the v1.11.0 milestone Apr 29, 2018
@heavywatal heavywatal deleted the heavywatal:print-returns-invisible-x branch Apr 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.