Skip to content

ARROW-10070: [C++][Compute] Implement var and std aggregate kernel - #8269

Closed
cyb70289 wants to merge 6 commits into
apache:masterfrom
cyb70289:agg-stdev
Closed

ARROW-10070: [C++][Compute] Implement var and std aggregate kernel#8269
cyb70289 wants to merge 6 commits into
apache:masterfrom
cyb70289:agg-stdev

Conversation

@cyb70289

@cyb70289cyb70289 commented Sep 25, 2020

Copy link
Copy Markdown
Contributor

No description provided.

@github-actions

github-actionsBot commented Sep 25, 2020

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-10070

@cyb70289

cyb70289 commented Sep 25, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Dev / Lint ci failure looks not related.

INFO:archery:Running Docker linter
apache-rat license violation: go/arrow/flight/Flight_grpc.pb.go
apache-rat license violation: go/arrow/flight/example_flight_server_test.go
Error: `docker-compose --file /home/runner/work/arrow/arrow/docker-compose.yml run --rm ubuntu-lint` exited with a non-zero exit code 1, see the process log above.

missing license header?
Will it related to pr #8175? @zeroshade@wesm

@kszucs

kszucs commented Sep 25, 2020•
edited
Loading

Copy link
Copy Markdown
Member

missing license header?

Yes, #8273 should fix that.

@jorisvandenbossche

jorisvandenbossche commented Sep 26, 2020

Copy link
Copy Markdown
Member

We might want to have an option to specify the denominator? (whether it is n or n - 1, to compute population vs sample standard deviation) As some examples, numpy has a ddof keyword, postgres or clickhouse have separate functions for both, julia has a corrected keyword.

@cyb70289

cyb70289 commented Sep 26, 2020

Copy link
Copy Markdown
ContributorAuthor

We might want to have an option to specify the denominator? (whether it is n or n - 1, to compute population vs sample standard deviation)

Thank you, will do.

@cyb70289

cyb70289 commented Sep 27, 2020

Copy link
Copy Markdown
ContributorAuthor

Added option ddof to control stdev divisor. Same as numpy.std.

@nealrichardson

nealrichardson commented Sep 28, 2020

Copy link
Copy Markdown
Member

Two notes:

  1. Naming: I've never seen this called stdev anywhere. stddev is common, in numpy and julia it's std, in R it's sd. Let's go with one of those. Maybe just add an extra "d"?
  2. Since sd = sqrt(var) (https://github.com/apache/arrow/pull/8269/files#diff-461bd7e445c2a190f1173ebdefa21002R106), would it make sense to implement variance (i.e. most of this patch), and then standard deviation as the sqrt of that? That way we get two kernels (or even three, if sqrt is exposed as a kernel too).

@cyb70289

cyb70289 commented Sep 29, 2020

Copy link
Copy Markdown
ContributorAuthor

Thanks @nealrichardson

1. Naming: I've never seen this called `stdev` anywhere. `stddev` is common, in numpy and julia it's `std`, in R it's `sd`. Let's go with one of those. Maybe just add an extra "d"?

Naming is always the hardest thing :) Looks std is used more often, and it's short.
AFAIK, stdev is used in excel (the most popular statistic software I guess? :)

2. Since `sd = sqrt(var)` (https://github.com/apache/arrow/pull/8269/files#diff-461bd7e445c2a190f1173ebdefa21002R106), would it make sense to implement variance (i.e. most of this patch), and then standard deviation as the sqrt of that? That way we get two kernels (or even three, if sqrt is exposed as a kernel too).

I also thought about the var kernel. Will update this patch to include it.

@cyb70289cyb70289 changed the title ARROW-10070: [C++][Compute] Implement stdev aggregate kernelARROW-10070: [C++][Compute] Implement var and std aggregate kernelSep 29, 2020
@cyb70289

cyb70289 commented Sep 29, 2020

Copy link
Copy Markdown
ContributorAuthor

Added variance kernel var. Renamed standard deviation kernel to std.

pitrou
pitrou reviewed Sep 29, 2020
View reviewed changes
Comment threaddocs/source/cpp/compute.rst Outdated

@pitroupitrouSep 29, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would rather "stddev" and "variance" respectively. "std" and "var" can easily be misunderstood, IMHO.

@cyb70289cyb70289Sep 30, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

pitrou
pitrou reviewed Sep 29, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/aggregate_var_std.cc Outdated
pitrou
pitrou reviewed Sep 29, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated

@pitroupitrouSep 29, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ironically, the fact that this is random identically-distributed data means that the slices will have similar means and variances. Perhaps making the array smaller would make the test more significant.

@cyb70289cyb70289Sep 30, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

pitrou
pitrou reviewed Sep 29, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated

@pitroupitrouSep 29, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also test with chunks of very different sizes? For example [1, 2, 3, 4, 5, 6, 7] and [8].

@cyb70289cyb70289Sep 30, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

pitrou
pitrou reviewed Sep 29, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated

@pitroupitrouSep 29, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@cyb70289cyb70289Sep 30, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

pitrou
pitrou reviewed Sep 29, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated

@pitroupitrouSep 29, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also test with ddof = 1? Though with such a large array size, the change may be rather small.

@cyb70289cyb70289Sep 30, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

arw2019
arw2019 reviewed Sep 30, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/api_aggregate.h Outdated

@arw2019arw2019Sep 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming suggestion: how about DdofOptions?

@cyb70289cyb70289Sep 30, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we will add options later, e.g., to ignore NaN

@jorisvandenbosschejorisvandenbosscheSep 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree there might be other options in the future. But with the renaming of the functions, maybe call it VarianceOptions instead?

@cyb70289cyb70289Sep 30, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of struggling as stddev kernel uses this same option.
Maybe export both VarianceOptions and StddevOptions and alias them as same type internally?

@pitroupitrouSep 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one is useful. Just VarianceOptions IMHO.

pitrou
pitrou approved these changes Sep 30, 2020
View reviewed changes

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, thank you @cyb70289

@jorisvandenbossche

jorisvandenbossche commented Sep 30, 2020

Copy link
Copy Markdown
Member

I suppose the behaviour with NaN is that any NaN in the input gives NaN as result? That might be worth adding a test for?

@pitrou

pitrou commented Sep 30, 2020

Copy link
Copy Markdown
Member

Since we're not doing anything special, it certainly will. I think that can be tackled in a separate JIRA, when adding a nan handling option perhaps.

@cyb70289
cyb70289 deleted the agg-stdev branch September 30, 2020 14:22
@asfimportasfimport mentioned this pull request Sep 30, 2020
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@pitroupitroupitrou approved these changes
+1 more reviewer
@arw2019arw2019arw2019 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@cyb70289@kszucs@jorisvandenbossche@nealrichardson@pitrou@arw2019