ARROW-10831: [C++][Compute] Implement quantile kernel - #8920

Closed
cyb70289 wants to merge 13 commits into
apache:masterfrom
cyb70289:quantile
Closed

ARROW-10831: [C++][Compute] Implement quantile kernel#8920
cyb70289 wants to merge 13 commits into
apache:masterfrom
cyb70289:quantile

Conversation

@cyb70289

@cyb70289cyb70289 commented Dec 15, 2020

Copy link
Copy Markdown
Contributor

Calculate the exact quantile by storing all values and partition
around quantile point at the end. This may require much memory.

As followup task, an approximate method without storing data points
will be implemented.

@github-actions

github-actionsBot commented Dec 15, 2020

Copy link
Copy Markdown

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

cyb70289
cyb70289 commented Dec 15, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/exec.cc Outdated
@cyb70289
cyb70289 marked this pull request as draft December 21, 2020 02:10
@cyb70289
cyb70289 marked this pull request as ready for review December 22, 2020 05:48
@cyb70289

cyb70289 commented Jan 8, 2021

Copy link
Copy Markdown
ContributorAuthor

@pitrou , will you review this patch? Thanks.

pitrou
pitrou reviewed Jan 11, 2021
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.

Thank you @cyb70289 ! Here are some comments.

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated

@pitroupitrouJan 11, 2021

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.

What happens if I ask for the median of [-Inf, Inf]?

@cyb70289cyb70289Jan 12, 2021

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.

Add a test to exercise this case.
Linear interpolated quantile will be -Inf. Numpy returns NaN.

@pitroupitrouJan 12, 2021

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.

Hmm, it's not very important, but NaN sounds more logical. What do you think?

@cyb70289cyb70289Jan 12, 2021

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.

Yes, will change.

@cyb70289cyb70289Jan 13, 2021•
edited
Loading

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.

Changed to NaN. Test case also updated.

Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
@cyb70289

cyb70289 commented Jan 12, 2021

Copy link
Copy Markdown
ContributorAuthor

CI failure is not related.

 The following tests FAILED:
38 - arrow-s3fs-test (Timeout)

@cyb70289

cyb70289 commented Jan 13, 2021

Copy link
Copy Markdown
ContributorAuthor

appveyor ci failure is about s3fs test

@pitrou

pitrou commented Jan 13, 2021

Copy link
Copy Markdown
Member

I'd rather defer this to 4.0 because QuantileOptions will also need to be exposed in Python.

@cyb70289

cyb70289 commented Jan 14, 2021

Copy link
Copy Markdown
ContributorAuthor

I'd rather defer this to 4.0 because QuantileOptions will also need to be exposed in Python.

No problem. Thanks.

@cyb70289

cyb70289 commented Jan 18, 2021

Copy link
Copy Markdown
ContributorAuthor

Exposed QuantileOptions to Python.
No hurry to merge.

In [1]: import pyarrow as pa
In [2]: import pyarrow.compute as pc
In [3]: arr = pa.array([1,2,3,4,5,6,7])
In [4]: option = pc.QuantileOptions(q=(0.1,0.3,0.5), interpolation='nearest')
In [5]: pc.call_function("quantile", [arr], option)
Out[5]: <pyarrow.lib.Int64Array object at 0x7f4fc0c345e8>
[
2,
3,
4
]

pitrou
pitrou approved these changes Jan 19, 2021
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.

Thank you very much @cyb70289 !

cyb70289and others added 13 commits January 19, 2021 18:35
@cyb70289@pitrou
ARROW-10831: [C++][Compute] Implement quantile kernel
33d3e5f
Calculate the exact quantile by storing all values and partition
around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points
will be implemented.
@cyb70289@pitrou
lower data point as maximal value left of higher data point
c33fdbe
@cyb70289@pitrou
use more stable linear interpolation
5664034
@cyb70289@pitrou
disable some floating point tests on mingw32
f2909a6
@cyb70289@pitrou
determine quantile type per interpolation method
24c29f3
@cyb70289@pitrou
calculate quantiles decreasingly to minimize partition size
93b1623
@cyb70289@pitrou
use datum to hold variant test values
9721602
@cyb70289@pitrou
address review comments
2b1e81e
@cyb70289@pitrou
fix index overflow in unit test
06f58b3
@cyb70289@pitrou
lerp(-inf, inf) = nan
79d9e42
@cyb70289@pitrou
document api as 'since 4.0.0'
79683e4
@cyb70289@pitrou
export QuantileOptions to python
0eb9cea
@pitrou
Improve Python binding
a24022e
@pitrou

pitrou commented Jan 19, 2021

Copy link
Copy Markdown
Member

Rebased and fixed conflict.

@cyb70289
cyb70289 deleted the quantile branch January 22, 2021 01:28
kszucs pushed a commit that referenced this pull request Jan 25, 2021
@cyb70289@pitrou@kszucs
ARROW-10831: [C++][Compute] Implement quantile kernel
1be28fb
Calculate the exact quantile by storing all values and partition
around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points
will be implemented.
Closes#8920 from cyb70289/quantile
Lead-authored-by: Yibo Cai <yibo.cai@arm.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@asfimportasfimport mentioned this pull request Apr 2, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitroupitroupitrou approved these changes

Assignees

No one assigned

Labels

Component: C++Component: Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cyb70289@pitrou
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
ARROW-10831: [C++][Compute] Implement quantile kernel - #8920
Closed
cyb70289 wants to merge 13 commits into
apache:masterfrom 
cyb70289:quantile
Closed
ARROW-10831: [C++][Compute] Implement quantile kernel#8920
cyb70289 wants to merge 13 commits into
apache:masterfrom 
cyb70289:quantile
Conversation
@cyb70289
@cyb70289cyb70289
commented
Dec 15, 2020

Copy link


Copy Markdown

Contributor
Calculate the exact quantile by storing all values and partition

around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points

will be implemented.
@github-actions
github-actionsBot
commented
Dec 15, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-10831
cyb70289
cyb70289
commented
Dec 15, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/exec.cc
Outdated

@cyb70289
cyb70289
marked this pull request as draft
December 21, 2020 02:10
@cyb70289
cyb70289
marked this pull request as ready for review
December 22, 2020 05:48
@cyb70289
cyb70289
commented
Jan 8, 2021

Copy link


Copy Markdown

ContributorAuthor
@pitrou , will you review this patch? Thanks.
pitrou
pitrou
reviewed
Jan 11, 2021
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.

Thank you @cyb70289 ! Here are some comments.
Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

@pitroupitrouJan 11, 2021

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.

What happens if I ask for the median of [-Inf, Inf]?
@cyb70289cyb70289Jan 12, 2021

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.

Add a test to exercise this case.

Linear interpolated quantile will be -Inf. Numpy returns NaN.
@pitroupitrouJan 12, 2021

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.

Hmm, it's not very important, but NaN sounds more logical. What do you think?
@cyb70289cyb70289Jan 12, 2021

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.

Yes, will change.
@cyb70289cyb70289Jan 13, 2021•

edited

Loading

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.

Changed to NaN. Test case also updated.
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

@cyb70289
cyb70289
commented
Jan 12, 2021

Copy link


Copy Markdown

ContributorAuthor
CI failure is not related.
 The following tests FAILED:
38 - arrow-s3fs-test (Timeout)

@cyb70289
cyb70289
commented
Jan 13, 2021

Copy link


Copy Markdown

ContributorAuthor
appveyor ci failure is about s3fs test
@pitrou
pitrou
commented
Jan 13, 2021

Copy link


Copy Markdown

Member
I'd rather defer this to 4.0 because QuantileOptions will also need to be exposed in Python.
@cyb70289
cyb70289
commented
Jan 14, 2021

Copy link


Copy Markdown

ContributorAuthor
I'd rather defer this to 4.0 because QuantileOptions will also need to be exposed in Python.
No problem. Thanks.
@cyb70289
cyb70289
commented
Jan 18, 2021

Copy link


Copy Markdown

ContributorAuthor
Exposed QuantileOptions to Python.

No hurry to merge.
In [1]: import pyarrow as pa
In [2]: import pyarrow.compute as pc
In [3]: arr = pa.array([1,2,3,4,5,6,7])
In [4]: option = pc.QuantileOptions(q=(0.1,0.3,0.5), interpolation='nearest')
In [5]: pc.call_function("quantile", [arr], option)
Out[5]: <pyarrow.lib.Int64Array object at 0x7f4fc0c345e8>
[
2,
3,
4
]

pitrou
pitrou
approved these changes
Jan 19, 2021
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.

Thank you very much @cyb70289 !
cyb70289and others
added 13 commits
January 19, 2021 18:35
@cyb70289@pitrou
ARROW-10831: [C++][Compute] Implement quantile kernel
33d3e5f
Calculate the exact quantile by storing all values and partition
around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points
will be implemented.
@cyb70289@pitrou
lower data point as maximal value left of higher data point
c33fdbe
@cyb70289@pitrou
use more stable linear interpolation
5664034
@cyb70289@pitrou
disable some floating point tests on mingw32
f2909a6
@cyb70289@pitrou
determine quantile type per interpolation method
24c29f3
@cyb70289@pitrou
calculate quantiles decreasingly to minimize partition size
93b1623
@cyb70289@pitrou
use datum to hold variant test values
9721602
@cyb70289@pitrou
address review comments
2b1e81e
@cyb70289@pitrou
fix index overflow in unit test
06f58b3
@cyb70289@pitrou
lerp(-inf, inf) = nan
79d9e42
@cyb70289@pitrou
document api as 'since 4.0.0'
79683e4
@cyb70289@pitrou
export QuantileOptions to python
0eb9cea
@pitrou
Improve Python binding
a24022e
@pitrou
pitrou
commented
Jan 19, 2021

Copy link


Copy Markdown

Member
Rebased and fixed conflict.
@cyb70289
cyb70289
deleted the

quantile

branch
January 22, 2021 01:28
kszucs pushed a commit
that referenced
this pull request
Jan 25, 2021
@cyb70289@pitrou@kszucs
ARROW-10831: [C++][Compute] Implement quantile kernel
1be28fb
Calculate the exact quantile by storing all values and partition
around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points
will be implemented.
Closes#8920 from cyb70289/quantile
Lead-authored-by: Yibo Cai <yibo.cai@arm.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@asfimportasfimport
mentioned this pull request
Apr 2, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@pitroupitroupitrou approved these changes

Assignees


No one assigned


Labels

Component: C++Component: Python

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@cyb70289@pitrou

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-10831: [C++][Compute] Implement quantile kernel - #8920
Closed
cyb70289 wants to merge 13 commits into
apache:masterfrom 
cyb70289:quantile
Closed
ARROW-10831: [C++][Compute] Implement quantile kernel#8920
cyb70289 wants to merge 13 commits into
apache:masterfrom 
cyb70289:quantile
Conversation
@cyb70289
@cyb70289cyb70289
commented
Dec 15, 2020

Copy link


Copy Markdown

Contributor
Calculate the exact quantile by storing all values and partition

around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points

will be implemented.
@github-actions
github-actionsBot
commented
Dec 15, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-10831
cyb70289
cyb70289
commented
Dec 15, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/exec.cc
Outdated

@cyb70289
cyb70289
marked this pull request as draft
December 21, 2020 02:10
@cyb70289
cyb70289
marked this pull request as ready for review
December 22, 2020 05:48
@cyb70289
cyb70289
commented
Jan 8, 2021

Copy link


Copy Markdown

ContributorAuthor
@pitrou , will you review this patch? Thanks.
pitrou
pitrou
reviewed
Jan 11, 2021
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.

Thank you @cyb70289 ! Here are some comments.
Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

@pitroupitrouJan 11, 2021

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.

What happens if I ask for the median of [-Inf, Inf]?
@cyb70289cyb70289Jan 12, 2021

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.

Add a test to exercise this case.

Linear interpolated quantile will be -Inf. Numpy returns NaN.
@pitroupitrouJan 12, 2021

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.

Hmm, it's not very important, but NaN sounds more logical. What do you think?
@cyb70289cyb70289Jan 12, 2021

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.

Yes, will change.
@cyb70289cyb70289Jan 13, 2021•

edited

Loading

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.

Changed to NaN. Test case also updated.
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

@cyb70289
cyb70289
commented
Jan 12, 2021

Copy link


Copy Markdown

ContributorAuthor
CI failure is not related.
 The following tests FAILED:
38 - arrow-s3fs-test (Timeout)

@cyb70289
cyb70289
commented
Jan 13, 2021

Copy link


Copy Markdown

ContributorAuthor
appveyor ci failure is about s3fs test
@pitrou
pitrou
commented
Jan 13, 2021

Copy link


Copy Markdown

Member
I'd rather defer this to 4.0 because QuantileOptions will also need to be exposed in Python.
@cyb70289
cyb70289
commented
Jan 14, 2021

Copy link


Copy Markdown

ContributorAuthor
I'd rather defer this to 4.0 because QuantileOptions will also need to be exposed in Python.
No problem. Thanks.
@cyb70289
cyb70289
commented
Jan 18, 2021

Copy link


Copy Markdown

ContributorAuthor
Exposed QuantileOptions to Python.

No hurry to merge.
In [1]: import pyarrow as pa
In [2]: import pyarrow.compute as pc
In [3]: arr = pa.array([1,2,3,4,5,6,7])
In [4]: option = pc.QuantileOptions(q=(0.1,0.3,0.5), interpolation='nearest')
In [5]: pc.call_function("quantile", [arr], option)
Out[5]: <pyarrow.lib.Int64Array object at 0x7f4fc0c345e8>
[
2,
3,
4
]

pitrou
pitrou
approved these changes
Jan 19, 2021
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.

Thank you very much @cyb70289 !
cyb70289and others
added 13 commits
January 19, 2021 18:35
@cyb70289@pitrou
ARROW-10831: [C++][Compute] Implement quantile kernel
33d3e5f
Calculate the exact quantile by storing all values and partition
around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points
will be implemented.
@cyb70289@pitrou
lower data point as maximal value left of higher data point
c33fdbe
@cyb70289@pitrou
use more stable linear interpolation
5664034
@cyb70289@pitrou
disable some floating point tests on mingw32
f2909a6
@cyb70289@pitrou
determine quantile type per interpolation method
24c29f3
@cyb70289@pitrou
calculate quantiles decreasingly to minimize partition size
93b1623
@cyb70289@pitrou
use datum to hold variant test values
9721602
@cyb70289@pitrou
address review comments
2b1e81e
@cyb70289@pitrou
fix index overflow in unit test
06f58b3
@cyb70289@pitrou
lerp(-inf, inf) = nan
79d9e42
@cyb70289@pitrou
document api as 'since 4.0.0'
79683e4
@cyb70289@pitrou
export QuantileOptions to python
0eb9cea
@pitrou
Improve Python binding
a24022e
@pitrou
pitrou
commented
Jan 19, 2021

Copy link


Copy Markdown

Member
Rebased and fixed conflict.
@cyb70289
cyb70289
deleted the

quantile

branch
January 22, 2021 01:28
kszucs pushed a commit
that referenced
this pull request
Jan 25, 2021
@cyb70289@pitrou@kszucs
ARROW-10831: [C++][Compute] Implement quantile kernel
1be28fb
Calculate the exact quantile by storing all values and partition
around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points
will be implemented.
Closes#8920 from cyb70289/quantile
Lead-authored-by: Yibo Cai <yibo.cai@arm.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@asfimportasfimport
mentioned this pull request
Apr 2, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@pitroupitroupitrou approved these changes

Assignees


No one assigned


Labels

Component: C++Component: Python

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@cyb70289@pitrou

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-10831: [C++][Compute] Implement quantile kernel - #8920
Closed
cyb70289 wants to merge 13 commits into
apache:masterfrom 
cyb70289:quantile
Closed
ARROW-10831: [C++][Compute] Implement quantile kernel#8920
cyb70289 wants to merge 13 commits into
apache:masterfrom 
cyb70289:quantile
Conversation
@cyb70289
@cyb70289cyb70289
commented
Dec 15, 2020

Copy link


Copy Markdown

Contributor
Calculate the exact quantile by storing all values and partition

around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points

will be implemented.
@github-actions
github-actionsBot
commented
Dec 15, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-10831
cyb70289
cyb70289
commented
Dec 15, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/exec.cc
Outdated

@cyb70289
cyb70289
marked this pull request as draft
December 21, 2020 02:10
@cyb70289
cyb70289
marked this pull request as ready for review
December 22, 2020 05:48
@cyb70289
cyb70289
commented
Jan 8, 2021

Copy link


Copy Markdown

ContributorAuthor
@pitrou , will you review this patch? Thanks.
pitrou
pitrou
reviewed
Jan 11, 2021
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.

Thank you @cyb70289 ! Here are some comments.
Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

@pitroupitrouJan 11, 2021

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.

What happens if I ask for the median of [-Inf, Inf]?
@cyb70289cyb70289Jan 12, 2021

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.

Add a test to exercise this case.

Linear interpolated quantile will be -Inf. Numpy returns NaN.
@pitroupitrouJan 12, 2021

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.

Hmm, it's not very important, but NaN sounds more logical. What do you think?
@cyb70289cyb70289Jan 12, 2021

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.

Yes, will change.
@cyb70289cyb70289Jan 13, 2021•

edited

Loading

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.

Changed to NaN. Test case also updated.
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

@cyb70289
cyb70289
commented
Jan 12, 2021

Copy link


Copy Markdown

ContributorAuthor
CI failure is not related.
 The following tests FAILED:
38 - arrow-s3fs-test (Timeout)

@cyb70289
cyb70289
commented
Jan 13, 2021

Copy link


Copy Markdown

ContributorAuthor
appveyor ci failure is about s3fs test
@pitrou
pitrou
commented
Jan 13, 2021

Copy link


Copy Markdown

Member
I'd rather defer this to 4.0 because QuantileOptions will also need to be exposed in Python.
@cyb70289
cyb70289
commented
Jan 14, 2021

Copy link


Copy Markdown

ContributorAuthor
I'd rather defer this to 4.0 because QuantileOptions will also need to be exposed in Python.
No problem. Thanks.
@cyb70289
cyb70289
commented
Jan 18, 2021

Copy link


Copy Markdown

ContributorAuthor
Exposed QuantileOptions to Python.

No hurry to merge.
In [1]: import pyarrow as pa
In [2]: import pyarrow.compute as pc
In [3]: arr = pa.array([1,2,3,4,5,6,7])
In [4]: option = pc.QuantileOptions(q=(0.1,0.3,0.5), interpolation='nearest')
In [5]: pc.call_function("quantile", [arr], option)
Out[5]: <pyarrow.lib.Int64Array object at 0x7f4fc0c345e8>
[
2,
3,
4
]

pitrou
pitrou
approved these changes
Jan 19, 2021
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.

Thank you very much @cyb70289 !
cyb70289and others
added 13 commits
January 19, 2021 18:35
@cyb70289@pitrou
ARROW-10831: [C++][Compute] Implement quantile kernel
33d3e5f
Calculate the exact quantile by storing all values and partition
around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points
will be implemented.
@cyb70289@pitrou
lower data point as maximal value left of higher data point
c33fdbe
@cyb70289@pitrou
use more stable linear interpolation
5664034
@cyb70289@pitrou
disable some floating point tests on mingw32
f2909a6
@cyb70289@pitrou
determine quantile type per interpolation method
24c29f3
@cyb70289@pitrou
calculate quantiles decreasingly to minimize partition size
93b1623
@cyb70289@pitrou
use datum to hold variant test values
9721602
@cyb70289@pitrou
address review comments
2b1e81e
@cyb70289@pitrou
fix index overflow in unit test
06f58b3
@cyb70289@pitrou
lerp(-inf, inf) = nan
79d9e42
@cyb70289@pitrou
document api as 'since 4.0.0'
79683e4
@cyb70289@pitrou
export QuantileOptions to python
0eb9cea
@pitrou
Improve Python binding
a24022e
@pitrou
pitrou
commented
Jan 19, 2021

Copy link


Copy Markdown

Member
Rebased and fixed conflict.
@cyb70289
cyb70289
deleted the

quantile

branch
January 22, 2021 01:28
kszucs pushed a commit
that referenced
this pull request
Jan 25, 2021
@cyb70289@pitrou@kszucs
ARROW-10831: [C++][Compute] Implement quantile kernel
1be28fb
Calculate the exact quantile by storing all values and partition
around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points
will be implemented.
Closes#8920 from cyb70289/quantile
Lead-authored-by: Yibo Cai <yibo.cai@arm.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@asfimportasfimport
mentioned this pull request
Apr 2, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@pitroupitroupitrou approved these changes

Assignees


No one assigned


Labels

Component: C++Component: Python

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@cyb70289@pitrou

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
ARROW-10831: [C++][Compute] Implement quantile kernel - #8920
Closed
cyb70289 wants to merge 13 commits into
apache:masterfrom 
cyb70289:quantile
Closed
ARROW-10831: [C++][Compute] Implement quantile kernel#8920
cyb70289 wants to merge 13 commits into
apache:masterfrom 
cyb70289:quantile
Conversation
@cyb70289
@cyb70289cyb70289
commented
Dec 15, 2020

Copy link


Copy Markdown

Contributor
Calculate the exact quantile by storing all values and partition

around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points

will be implemented.
@github-actions
github-actionsBot
commented
Dec 15, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-10831
cyb70289
cyb70289
commented
Dec 15, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/exec.cc
Outdated

@cyb70289
cyb70289
marked this pull request as draft
December 21, 2020 02:10
@cyb70289
cyb70289
marked this pull request as ready for review
December 22, 2020 05:48
@cyb70289
cyb70289
commented
Jan 8, 2021

Copy link


Copy Markdown

ContributorAuthor
@pitrou , will you review this patch? Thanks.
pitrou
pitrou
reviewed
Jan 11, 2021
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.

Thank you @cyb70289 ! Here are some comments.
Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

@pitroupitrouJan 11, 2021

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.

What happens if I ask for the median of [-Inf, Inf]?
@cyb70289cyb70289Jan 12, 2021

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.

Add a test to exercise this case.

Linear interpolated quantile will be -Inf. Numpy returns NaN.
@pitroupitrouJan 12, 2021

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.

Hmm, it's not very important, but NaN sounds more logical. What do you think?
@cyb70289cyb70289Jan 12, 2021

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.

Yes, will change.
@cyb70289cyb70289Jan 13, 2021•

edited

Loading

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.

Changed to NaN. Test case also updated.
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

@cyb70289
cyb70289
commented
Jan 12, 2021

Copy link


Copy Markdown

ContributorAuthor
CI failure is not related.
 The following tests FAILED:
38 - arrow-s3fs-test (Timeout)

@cyb70289
cyb70289
commented
Jan 13, 2021

Copy link


Copy Markdown

ContributorAuthor
appveyor ci failure is about s3fs test
@pitrou
pitrou
commented
Jan 13, 2021

Copy link


Copy Markdown

Member
I'd rather defer this to 4.0 because QuantileOptions will also need to be exposed in Python.
@cyb70289
cyb70289
commented
Jan 14, 2021

Copy link


Copy Markdown

ContributorAuthor
I'd rather defer this to 4.0 because QuantileOptions will also need to be exposed in Python.
No problem. Thanks.
@cyb70289
cyb70289
commented
Jan 18, 2021

Copy link


Copy Markdown

ContributorAuthor
Exposed QuantileOptions to Python.

No hurry to merge.
In [1]: import pyarrow as pa
In [2]: import pyarrow.compute as pc
In [3]: arr = pa.array([1,2,3,4,5,6,7])
In [4]: option = pc.QuantileOptions(q=(0.1,0.3,0.5), interpolation='nearest')
In [5]: pc.call_function("quantile", [arr], option)
Out[5]: <pyarrow.lib.Int64Array object at 0x7f4fc0c345e8>
[
2,
3,
4
]

pitrou
pitrou
approved these changes
Jan 19, 2021
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.

Thank you very much @cyb70289 !
cyb70289and others
added 13 commits
January 19, 2021 18:35
@cyb70289@pitrou
ARROW-10831: [C++][Compute] Implement quantile kernel
33d3e5f
Calculate the exact quantile by storing all values and partition
around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points
will be implemented.
@cyb70289@pitrou
lower data point as maximal value left of higher data point
c33fdbe
@cyb70289@pitrou
use more stable linear interpolation
5664034
@cyb70289@pitrou
disable some floating point tests on mingw32
f2909a6
@cyb70289@pitrou
determine quantile type per interpolation method
24c29f3
@cyb70289@pitrou
calculate quantiles decreasingly to minimize partition size
93b1623
@cyb70289@pitrou
use datum to hold variant test values
9721602
@cyb70289@pitrou
address review comments
2b1e81e
@cyb70289@pitrou
fix index overflow in unit test
06f58b3
@cyb70289@pitrou
lerp(-inf, inf) = nan
79d9e42
@cyb70289@pitrou
document api as 'since 4.0.0'
79683e4
@cyb70289@pitrou
export QuantileOptions to python
0eb9cea
@pitrou
Improve Python binding
a24022e
@pitrou
pitrou
commented
Jan 19, 2021

Copy link


Copy Markdown

Member
Rebased and fixed conflict.
@cyb70289
cyb70289
deleted the

quantile

branch
January 22, 2021 01:28
kszucs pushed a commit
that referenced
this pull request
Jan 25, 2021
@cyb70289@pitrou@kszucs
ARROW-10831: [C++][Compute] Implement quantile kernel
1be28fb
Calculate the exact quantile by storing all values and partition
around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points
will be implemented.
Closes#8920 from cyb70289/quantile
Lead-authored-by: Yibo Cai <yibo.cai@arm.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@asfimportasfimport
mentioned this pull request
Apr 2, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@pitroupitroupitrou approved these changes

Assignees


No one assigned


Labels

Component: C++Component: Python

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@cyb70289@pitrou

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-10831: [C++][Compute] Implement quantile kernel - #8920
Closed
cyb70289 wants to merge 13 commits into
apache:masterfrom 
cyb70289:quantile
Closed
ARROW-10831: [C++][Compute] Implement quantile kernel#8920
cyb70289 wants to merge 13 commits into
apache:masterfrom 
cyb70289:quantile
Conversation
@cyb70289
@cyb70289cyb70289
commented
Dec 15, 2020

Copy link


Copy Markdown

Contributor
Calculate the exact quantile by storing all values and partition

around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points

will be implemented.
@github-actions
github-actionsBot
commented
Dec 15, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-10831
cyb70289
cyb70289
commented
Dec 15, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/exec.cc
Outdated

@cyb70289
cyb70289
marked this pull request as draft
December 21, 2020 02:10
@cyb70289
cyb70289
marked this pull request as ready for review
December 22, 2020 05:48
@cyb70289
cyb70289
commented
Jan 8, 2021

Copy link


Copy Markdown

ContributorAuthor
@pitrou , will you review this patch? Thanks.
pitrou
pitrou
reviewed
Jan 11, 2021
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.

Thank you @cyb70289 ! Here are some comments.
Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

@pitroupitrouJan 11, 2021

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.

What happens if I ask for the median of [-Inf, Inf]?
@cyb70289cyb70289Jan 12, 2021

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.

Add a test to exercise this case.

Linear interpolated quantile will be -Inf. Numpy returns NaN.
@pitroupitrouJan 12, 2021

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.

Hmm, it's not very important, but NaN sounds more logical. What do you think?
@cyb70289cyb70289Jan 12, 2021

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.

Yes, will change.
@cyb70289cyb70289Jan 13, 2021•

edited

Loading

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.

Changed to NaN. Test case also updated.
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

@cyb70289
cyb70289
commented
Jan 12, 2021

Copy link


Copy Markdown

ContributorAuthor
CI failure is not related.
 The following tests FAILED:
38 - arrow-s3fs-test (Timeout)

@cyb70289
cyb70289
commented
Jan 13, 2021

Copy link


Copy Markdown

ContributorAuthor
appveyor ci failure is about s3fs test
@pitrou
pitrou
commented
Jan 13, 2021

Copy link


Copy Markdown

Member
I'd rather defer this to 4.0 because QuantileOptions will also need to be exposed in Python.
@cyb70289
cyb70289
commented
Jan 14, 2021

Copy link


Copy Markdown

ContributorAuthor
I'd rather defer this to 4.0 because QuantileOptions will also need to be exposed in Python.
No problem. Thanks.
@cyb70289
cyb70289
commented
Jan 18, 2021

Copy link


Copy Markdown

ContributorAuthor
Exposed QuantileOptions to Python.

No hurry to merge.
In [1]: import pyarrow as pa
In [2]: import pyarrow.compute as pc
In [3]: arr = pa.array([1,2,3,4,5,6,7])
In [4]: option = pc.QuantileOptions(q=(0.1,0.3,0.5), interpolation='nearest')
In [5]: pc.call_function("quantile", [arr], option)
Out[5]: <pyarrow.lib.Int64Array object at 0x7f4fc0c345e8>
[
2,
3,
4
]

pitrou
pitrou
approved these changes
Jan 19, 2021
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.

Thank you very much @cyb70289 !
cyb70289and others
added 13 commits
January 19, 2021 18:35
@cyb70289@pitrou
ARROW-10831: [C++][Compute] Implement quantile kernel
33d3e5f
Calculate the exact quantile by storing all values and partition
around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points
will be implemented.
@cyb70289@pitrou
lower data point as maximal value left of higher data point
c33fdbe
@cyb70289@pitrou
use more stable linear interpolation
5664034
@cyb70289@pitrou
disable some floating point tests on mingw32
f2909a6
@cyb70289@pitrou
determine quantile type per interpolation method
24c29f3
@cyb70289@pitrou
calculate quantiles decreasingly to minimize partition size
93b1623
@cyb70289@pitrou
use datum to hold variant test values
9721602
@cyb70289@pitrou
address review comments
2b1e81e
@cyb70289@pitrou
fix index overflow in unit test
06f58b3
@cyb70289@pitrou
lerp(-inf, inf) = nan
79d9e42
@cyb70289@pitrou
document api as 'since 4.0.0'
79683e4
@cyb70289@pitrou
export QuantileOptions to python
0eb9cea
@pitrou
Improve Python binding
a24022e
@pitrou
pitrou
commented
Jan 19, 2021

Copy link


Copy Markdown

Member
Rebased and fixed conflict.
@cyb70289
cyb70289
deleted the

quantile

branch
January 22, 2021 01:28
kszucs pushed a commit
that referenced
this pull request
Jan 25, 2021
@cyb70289@pitrou@kszucs
ARROW-10831: [C++][Compute] Implement quantile kernel
1be28fb
Calculate the exact quantile by storing all values and partition
around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points
will be implemented.
Closes#8920 from cyb70289/quantile
Lead-authored-by: Yibo Cai <yibo.cai@arm.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@asfimportasfimport
mentioned this pull request
Apr 2, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@pitroupitroupitrou approved these changes

Assignees


No one assigned


Labels

Component: C++Component: Python

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@cyb70289@pitrou

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-10831: [C++][Compute] Implement quantile kernel - #8920
Closed
cyb70289 wants to merge 13 commits into
apache:masterfrom 
cyb70289:quantile
Closed
ARROW-10831: [C++][Compute] Implement quantile kernel#8920
cyb70289 wants to merge 13 commits into
apache:masterfrom 
cyb70289:quantile
Conversation
@cyb70289
@cyb70289cyb70289
commented
Dec 15, 2020

Copy link


Copy Markdown

Contributor
Calculate the exact quantile by storing all values and partition

around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points

will be implemented.
@github-actions
github-actionsBot
commented
Dec 15, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-10831
cyb70289
cyb70289
commented
Dec 15, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/exec.cc
Outdated

@cyb70289
cyb70289
marked this pull request as draft
December 21, 2020 02:10
@cyb70289
cyb70289
marked this pull request as ready for review
December 22, 2020 05:48
@cyb70289
cyb70289
commented
Jan 8, 2021

Copy link


Copy Markdown

ContributorAuthor
@pitrou , will you review this patch? Thanks.
pitrou
pitrou
reviewed
Jan 11, 2021
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.

Thank you @cyb70289 ! Here are some comments.
Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

@pitroupitrouJan 11, 2021

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.

What happens if I ask for the median of [-Inf, Inf]?
@cyb70289cyb70289Jan 12, 2021

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.

Add a test to exercise this case.

Linear interpolated quantile will be -Inf. Numpy returns NaN.
@pitroupitrouJan 12, 2021

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.

Hmm, it's not very important, but NaN sounds more logical. What do you think?
@cyb70289cyb70289Jan 12, 2021

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.

Yes, will change.
@cyb70289cyb70289Jan 13, 2021•

edited

Loading

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.

Changed to NaN. Test case also updated.
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

@cyb70289
cyb70289
commented
Jan 12, 2021

Copy link


Copy Markdown

ContributorAuthor
CI failure is not related.
 The following tests FAILED:
38 - arrow-s3fs-test (Timeout)

@cyb70289
cyb70289
commented
Jan 13, 2021

Copy link


Copy Markdown

ContributorAuthor
appveyor ci failure is about s3fs test
@pitrou
pitrou
commented
Jan 13, 2021

Copy link


Copy Markdown

Member
I'd rather defer this to 4.0 because QuantileOptions will also need to be exposed in Python.
@cyb70289
cyb70289
commented
Jan 14, 2021

Copy link


Copy Markdown

ContributorAuthor
I'd rather defer this to 4.0 because QuantileOptions will also need to be exposed in Python.
No problem. Thanks.
@cyb70289
cyb70289
commented
Jan 18, 2021

Copy link


Copy Markdown

ContributorAuthor
Exposed QuantileOptions to Python.

No hurry to merge.
In [1]: import pyarrow as pa
In [2]: import pyarrow.compute as pc
In [3]: arr = pa.array([1,2,3,4,5,6,7])
In [4]: option = pc.QuantileOptions(q=(0.1,0.3,0.5), interpolation='nearest')
In [5]: pc.call_function("quantile", [arr], option)
Out[5]: <pyarrow.lib.Int64Array object at 0x7f4fc0c345e8>
[
2,
3,
4
]

pitrou
pitrou
approved these changes
Jan 19, 2021
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.

Thank you very much @cyb70289 !
cyb70289and others
added 13 commits
January 19, 2021 18:35
@cyb70289@pitrou
ARROW-10831: [C++][Compute] Implement quantile kernel
33d3e5f
Calculate the exact quantile by storing all values and partition
around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points
will be implemented.
@cyb70289@pitrou
lower data point as maximal value left of higher data point
c33fdbe
@cyb70289@pitrou
use more stable linear interpolation
5664034
@cyb70289@pitrou
disable some floating point tests on mingw32
f2909a6
@cyb70289@pitrou
determine quantile type per interpolation method
24c29f3
@cyb70289@pitrou
calculate quantiles decreasingly to minimize partition size
93b1623
@cyb70289@pitrou
use datum to hold variant test values
9721602
@cyb70289@pitrou
address review comments
2b1e81e
@cyb70289@pitrou
fix index overflow in unit test
06f58b3
@cyb70289@pitrou
lerp(-inf, inf) = nan
79d9e42
@cyb70289@pitrou
document api as 'since 4.0.0'
79683e4
@cyb70289@pitrou
export QuantileOptions to python
0eb9cea
@pitrou
Improve Python binding
a24022e
@pitrou
pitrou
commented
Jan 19, 2021

Copy link


Copy Markdown

Member
Rebased and fixed conflict.
@cyb70289
cyb70289
deleted the

quantile

branch
January 22, 2021 01:28
kszucs pushed a commit
that referenced
this pull request
Jan 25, 2021
@cyb70289@pitrou@kszucs
ARROW-10831: [C++][Compute] Implement quantile kernel
1be28fb
Calculate the exact quantile by storing all values and partition
around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points
will be implemented.
Closes#8920 from cyb70289/quantile
Lead-authored-by: Yibo Cai <yibo.cai@arm.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@asfimportasfimport
mentioned this pull request
Apr 2, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@pitroupitroupitrou approved these changes

Assignees


No one assigned


Labels

Component: C++Component: Python

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@cyb70289@pitrou

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
ARROW-10831: [C++][Compute] Implement quantile kernel - #8920
Closed
cyb70289 wants to merge 13 commits into
apache:masterfrom 
cyb70289:quantile
Closed
ARROW-10831: [C++][Compute] Implement quantile kernel#8920
cyb70289 wants to merge 13 commits into
apache:masterfrom 
cyb70289:quantile
Conversation
@cyb70289
@cyb70289cyb70289
commented
Dec 15, 2020

Copy link


Copy Markdown

Contributor
Calculate the exact quantile by storing all values and partition

around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points

will be implemented.
@github-actions
github-actionsBot
commented
Dec 15, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-10831
cyb70289
cyb70289
commented
Dec 15, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/exec.cc
Outdated

@cyb70289
cyb70289
marked this pull request as draft
December 21, 2020 02:10
@cyb70289
cyb70289
marked this pull request as ready for review
December 22, 2020 05:48
@cyb70289
cyb70289
commented
Jan 8, 2021

Copy link


Copy Markdown

ContributorAuthor
@pitrou , will you review this patch? Thanks.
pitrou
pitrou
reviewed
Jan 11, 2021
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.

Thank you @cyb70289 ! Here are some comments.
Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_quantile.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

@pitroupitrouJan 11, 2021

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.

What happens if I ask for the median of [-Inf, Inf]?
@cyb70289cyb70289Jan 12, 2021

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.

Add a test to exercise this case.

Linear interpolated quantile will be -Inf. Numpy returns NaN.
@pitroupitrouJan 12, 2021

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.

Hmm, it's not very important, but NaN sounds more logical. What do you think?
@cyb70289cyb70289Jan 12, 2021

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.

Yes, will change.
@cyb70289cyb70289Jan 13, 2021•

edited

Loading

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.

Changed to NaN. Test case also updated.
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc
Outdated

@cyb70289
cyb70289
commented
Jan 12, 2021

Copy link


Copy Markdown

ContributorAuthor
CI failure is not related.
 The following tests FAILED:
38 - arrow-s3fs-test (Timeout)

@cyb70289
cyb70289
commented
Jan 13, 2021

Copy link


Copy Markdown

ContributorAuthor
appveyor ci failure is about s3fs test
@pitrou
pitrou
commented
Jan 13, 2021

Copy link


Copy Markdown

Member
I'd rather defer this to 4.0 because QuantileOptions will also need to be exposed in Python.
@cyb70289
cyb70289
commented
Jan 14, 2021

Copy link


Copy Markdown

ContributorAuthor
I'd rather defer this to 4.0 because QuantileOptions will also need to be exposed in Python.
No problem. Thanks.
@cyb70289
cyb70289
commented
Jan 18, 2021

Copy link


Copy Markdown

ContributorAuthor
Exposed QuantileOptions to Python.

No hurry to merge.
In [1]: import pyarrow as pa
In [2]: import pyarrow.compute as pc
In [3]: arr = pa.array([1,2,3,4,5,6,7])
In [4]: option = pc.QuantileOptions(q=(0.1,0.3,0.5), interpolation='nearest')
In [5]: pc.call_function("quantile", [arr], option)
Out[5]: <pyarrow.lib.Int64Array object at 0x7f4fc0c345e8>
[
2,
3,
4
]

pitrou
pitrou
approved these changes
Jan 19, 2021
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.

Thank you very much @cyb70289 !
cyb70289and others
added 13 commits
January 19, 2021 18:35
@cyb70289@pitrou
ARROW-10831: [C++][Compute] Implement quantile kernel
33d3e5f
Calculate the exact quantile by storing all values and partition
around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points
will be implemented.
@cyb70289@pitrou
lower data point as maximal value left of higher data point
c33fdbe
@cyb70289@pitrou
use more stable linear interpolation
5664034
@cyb70289@pitrou
disable some floating point tests on mingw32
f2909a6
@cyb70289@pitrou
determine quantile type per interpolation method
24c29f3
@cyb70289@pitrou
calculate quantiles decreasingly to minimize partition size
93b1623
@cyb70289@pitrou
use datum to hold variant test values
9721602
@cyb70289@pitrou
address review comments
2b1e81e
@cyb70289@pitrou
fix index overflow in unit test
06f58b3
@cyb70289@pitrou
lerp(-inf, inf) = nan
79d9e42
@cyb70289@pitrou
document api as 'since 4.0.0'
79683e4
@cyb70289@pitrou
export QuantileOptions to python
0eb9cea
@pitrou
Improve Python binding
a24022e
@pitrou
pitrou
commented
Jan 19, 2021

Copy link


Copy Markdown

Member
Rebased and fixed conflict.
@cyb70289
cyb70289
deleted the

quantile

branch
January 22, 2021 01:28
kszucs pushed a commit
that referenced
this pull request
Jan 25, 2021
@cyb70289@pitrou@kszucs
ARROW-10831: [C++][Compute] Implement quantile kernel
1be28fb
Calculate the exact quantile by storing all values and partition
around quantile point at the end. This may require much memory.
As followup task, an approximate method without storing data points
will be implemented.
Closes#8920 from cyb70289/quantile
Lead-authored-by: Yibo Cai <yibo.cai@arm.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@asfimportasfimport
mentioned this pull request
Apr 2, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@pitroupitroupitrou approved these changes

Assignees


No one assigned


Labels

Component: C++Component: Python

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@cyb70289@pitrou