Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
cyb70289 wants to merge 6 commits into from
Closed

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

cyb70289 wants to merge 6 commits into from

Conversation

@cyb70289
Copy link
Contributor

@cyb70289 cyb70289 commented Sep 25, 2020

No description provided.

@github-actions
Copy link

github-actions bot commented Sep 25, 2020

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

@cyb70289
Copy link
Contributor Author

cyb70289 commented Sep 25, 2020 •
edited
Loading

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

kszucs commented Sep 25, 2020 •
edited
Loading

missing license header?

Yes, #8273 should fix that.

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Sep 26, 2020

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

cyb70289 commented Sep 26, 2020

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

cyb70289 commented Sep 27, 2020

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

@nealrichardson
Copy link
Member

nealrichardson commented Sep 28, 2020

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

cyb70289 commented Sep 29, 2020

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.

@cyb70289 cyb70289 changed the title ARROW-10070: [C++][Compute] Implement stdev aggregate kernel ARROW-10070: [C++][Compute] Implement var and std aggregate kernel Sep 29, 2020
@cyb70289
Copy link
Contributor Author

cyb70289 commented Sep 29, 2020

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

pitrou
pitrou reviewed Sep 29, 2020
View reviewed changes
docs/source/cpp/compute.rst Outdated
Copy link
Member

@pitrou pitrou Sep 29, 2020

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.

Copy link
Contributor Author

@cyb70289 cyb70289 Sep 30, 2020

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
pitrou
pitrou reviewed Sep 29, 2020
View reviewed changes
cpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
Copy link
Member

@pitrou pitrou Sep 29, 2020

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.

Copy link
Contributor Author

@cyb70289 cyb70289 Sep 30, 2020

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
cpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
Copy link
Member

@pitrou pitrou Sep 29, 2020

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].

Copy link
Contributor Author

@cyb70289 cyb70289 Sep 30, 2020

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
cpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
Copy link
Member

@pitrou pitrou Sep 29, 2020

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

@cyb70289 cyb70289 Sep 30, 2020

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
cpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
Copy link
Member

@pitrou pitrou Sep 29, 2020

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.

Copy link
Contributor Author

@cyb70289 cyb70289 Sep 30, 2020

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
cpp/src/arrow/compute/api_aggregate.h Outdated
Copy link
Contributor

@arw2019 arw2019 Sep 30, 2020

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?

Copy link
Contributor Author

@cyb70289 cyb70289 Sep 30, 2020

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

Copy link
Member

@jorisvandenbossche jorisvandenbossche Sep 30, 2020

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?

Copy link
Contributor Author

@cyb70289 cyb70289 Sep 30, 2020

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?

Copy link
Member

@pitrou pitrou Sep 30, 2020

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

@pitrou pitrou left a comment

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

jorisvandenbossche commented Sep 30, 2020

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

pitrou commented Sep 30, 2020

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.

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

Reviewers

@jorisvandenbossche jorisvandenbossche jorisvandenbossche left review comments

@pitrou pitrou pitrou approved these changes

+1 more reviewer

@arw2019 arw2019 arw2019 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