Closed hms1 closed 2 years ago
I noticed that there is a failing test but this seems to be caused by fcdf240
@michaelquinn32 What do you think? as.data.frame() is actually also less preferred than data.frame().
Since we use tibbles essentially throughout it probably makes sense to go all in.
@hms1 Yes we know there is a test issue (you can even see it in the main build).
Can you update your branch to PR against the current version of develop and try the checks again?
Otherwise, we can look to patch the tests to resolve the issue.
Thanks!
Also, mind updating the NEWS and adding yourself as a contributor in the DESCRIPTION? Really appreciate you taking the effort to sort these things out!
While agree with this minor change with meaningful impact, I'm still wondering the following: If the data has thousands of columns and the time is growing exponentially with columns, would it make sense to chunk? I don't know if it really is exponential but I feel like this is a use case that we really didn't consider. For example, print doesn't (i don't think) have pagination
By the way, in testing in order to isolate the issue of the impact of many columns I only used 100 rows. Also it might make sense to look at whether the types (or number of types) of columns have an impact.
Anyway, I think let's start a separate issue about increasing speed.
This very small PR replaces
as.data.frame
withas_tibble
inside thereshape_skimmed
function. This alone has a significant impact on performance, particularly for large datasets:as.data.frame
is known to be slow (see here) and in combination with increased calls toreshape_skimmed
as variables are added leads to an exponential degradation in performance with dataset size.To demonstrate, build a moderately sized dataset:
Using the previous
develop
branch is almost an order of magnitude slower:The difference becomes greater as variables are added.
A further rework/simplification of the
skim_by_type
methods can speed things up further still