Closed cjprybol closed 6 years ago
Looks nice. Can you explain (here and in the commit message) what the change consists in? Why would bypassing combine
improve performance so much? Should combine
be changed too?
I'm not sure how much of the efficiency comes from bypassing map
and how much comes from bypassing combine
. map
will construct DataTables
for each GroupApplied
which adds the overhead of the DataTable
constructor multiple times. combine
then vcats
and hcats
all of those DataTables
together, adding additional checks, assertions, and overhead.
I don't want to touch combine
here because I don't know how to support anonymous functions that return DataTables
as shown in by
examples without it. If I can figure out how to support that with less overhead then we might be able to simplify some of this code more. I think it's safe to remove the calls to map
and combine
in aggregate
because only one of the two aggregate
methods (the one that grouped by column before hand) used map
and combine
. With these changes, users will still have by
for more complex split-apply-combine queries, and aggregate
is a simpler and more efficient alternative.
aggregate
Bump!
@cjprybol I suppose this should be backported to DataFrames?
Yes, thanks for the reminder! I should also put these benchmarks into the beginnings of a PkgBenchmark suite. I think there are benchmarks we have scattered around other PRs in DataTables too. I couldn't figure out how to use PkgBenchmark the last time I tried, but this comment covers most of the process
Example code
^ The above loads this file into a datatable that looks like this
I wanted to ask how many transcripts there are for each gene, which can be accomplished by grouping by gene and taking the length of each group.
The 199324 transcripts group down to 58219 genes, but the current master code chokes up. I remember letting it run for something like 15 minutes before giving up. This time I got a segfault when trying to group on the first ~50% of the datatable.
Results
I was satisfied with that improvement for my particular case, although I'd be happy to run this on other cases if anyone would like to propose other benchmarks. Consider this an RFC until I write tests.
this file =
ftp://ftp.sanger.ac.uk/pub/gencode/Gencode_human/release_26/gencode.v26.transcripts.fa.gz
(for some reason markdown hyperlinks aren't working with that url, maybe because it's ftp?)