You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "lorentzenchr (via GitHub)" <gi...@apache.org> on 2023/08/25 08:46:47 UTC
[GitHub] [arrow] lorentzenchr opened a new pull request, #37380: DOC clearer description of q argument of quantiles
lorentzenchr opened a new pull request, #37380:
URL: https://github.com/apache/arrow/pull/37380
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
Documention of the `q` parameter of quantiles is made more precise.
### What changes are included in this PR?
Only documentation changes.
### Are these changes tested?
No
### Are there any user-facing changes?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] conbench-apache-arrow[bot] commented on pull request #37380: MINOR: [C++][Python] [Docs] clearer description of q argument of quantiles
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #37380:
URL: https://github.com/apache/arrow/pull/37380#issuecomment-1721817050
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit d8ab9a802227468976958b109c08c1ca9637a7e8.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/16841266917) has more details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on pull request #37380: MINOR: [C++][Python] [Docs] clearer description of q argument of quantiles
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #37380:
URL: https://github.com/apache/arrow/pull/37380#issuecomment-1720318177
> Can I do anything else here to finish it?
The only thing needed is for a committer to merge the PR. Pinging @pitrou.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lorentzenchr commented on a diff in pull request #37380: MINOR: [C++][Python] [Docs] clearer description of q argument of quantiles
Posted by "lorentzenchr (via GitHub)" <gi...@apache.org>.
lorentzenchr commented on code in PR #37380:
URL: https://github.com/apache/arrow/pull/37380#discussion_r1311872692
##########
python/pyarrow/_compute.pyx:
##########
@@ -2145,7 +2145,8 @@ class QuantileOptions(_QuantileOptions):
Parameters
----------
q : double or sequence of double, default 0.5
- Quantiles to compute. All values must be in [0, 1].
+ Probability levels of the quantiles to compute. All values must be in
Review Comment:
Please see
- R (base!) [quantile](https://stat.ethz.ch/R-manual/R-devel/library/stats/html/quantile.html) argument `probs`
- Julia [quantile](https://docs.julialang.org/en/v1/stdlib/Statistics/#Statistics.quantile)
> quantile(s) ... at a specified probability or vector or tuple of probabilities `p`
- https://github.com/numpy/numpy/pull/23529
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #37380: MINOR: [C++][Python] [Docs] clearer description of q argument of quantiles
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #37380:
URL: https://github.com/apache/arrow/pull/37380#discussion_r1312038163
##########
python/pyarrow/_compute.pyx:
##########
@@ -2145,7 +2145,8 @@ class QuantileOptions(_QuantileOptions):
Parameters
----------
q : double or sequence of double, default 0.5
- Quantiles to compute. All values must be in [0, 1].
+ Probability levels of the quantiles to compute. All values must be in
Review Comment:
"Probability level" is new terminology and could mean anything -- that is why it seems to work as a replacement for the ambiguous "quantile" here.
"quantile" can refer to the calculated value and the desired quantile as a number between `0.0` and `1.0`. Isn't the comment saying they are between 0.0 and 1.0 enough to disambiguate?
The original t-Digest paper refers to these as quantiles [1]
> With the t-digest, accuracy for estimating the q quantile is nearly constant relative to q(1 − q). This is in contrast to earlier algorithms which had errors independent of q. The relative error bound of the t-digest is convenient when computing quantiles for q near 0 or 1 as is commonly required. As with the Q-digest algorithm, the accuracy/size trade-off for the t-digest can be controlled by setting a single compression parameter δ with the amount of memory required proportional only to Θ(δ).
I see you are referring to R's `quantile` documentation that uses "probs". I would argue that they only did it because passing "quantile" to a "quantile" function would be weird. In the case of t-Digest, calling this argument "quantile" is more informative than "probability level". Probability of what?
If you really want to rename this to something that diverges from the the original t-Digest paper, I suggest the term "fractions" that has been adoped by Redis [2] and Druid [3].
[1] https://arxiv.org/pdf/1902.04023.pdf
[2] https://redis.io/docs/data-types/probabilistic/t-digest/
[3] https://druid.apache.org/docs/latest/development/extensions-contrib/tdigestsketch-quantiles/
##########
python/pyarrow/_compute.pyx:
##########
@@ -2145,7 +2145,8 @@ class QuantileOptions(_QuantileOptions):
Parameters
----------
q : double or sequence of double, default 0.5
- Quantiles to compute. All values must be in [0, 1].
+ Probability levels of the quantiles to compute. All values must be in
Review Comment:
"Probability level" is new terminology and could mean anything -- that is why it seems to work as a replacement for the ambiguous "quantile" here.
"quantile" can refer to the calculated value and the desired quantile as a number between `0.0` and `1.0`. Isn't the comment saying they are between 0.0 and 1.0 enough to disambiguate?
The original t-Digest paper refers to these as quantiles [1]
> With the t-digest, accuracy for estimating the q quantile is nearly constant relative to q(1 − q). This is in contrast to earlier algorithms which had errors independent of q. The relative error bound of the t-digest is convenient when computing quantiles for q near 0 or 1 as is commonly required. As with the Q-digest algorithm, the accuracy/size trade-off for the t-digest can be controlled by setting a single compression parameter δ with the amount of memory required proportional only to Θ(δ).
I see you are referring to R's `quantile` documentation that uses "probs". I would argue that they only did it because passing "quantile" to a "quantile" function would be weird. In the case of t-Digest, calling this argument "quantile" is more informative than "probability level". Probability of what?
If you really want to rename this to something that diverges from the the original t-Digest paper, I suggest the term "fractions" that has been adoped by Redis [2] and Druid [3].
[1] https://arxiv.org/pdf/1902.04023.pdf
[2] https://redis.io/docs/data-types/probabilistic/t-digest/
[3] https://druid.apache.org/docs/latest/development/extensions-contrib/tdigestsketch-quantiles/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #37380: MINOR: [C++][Python] [Docs] clearer description of q argument of quantiles
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #37380:
URL: https://github.com/apache/arrow/pull/37380#discussion_r1311649328
##########
python/pyarrow/_compute.pyx:
##########
@@ -2145,7 +2145,8 @@ class QuantileOptions(_QuantileOptions):
Parameters
----------
q : double or sequence of double, default 0.5
- Quantiles to compute. All values must be in [0, 1].
+ Probability levels of the quantiles to compute. All values must be in
Review Comment:
I'm not sure "probability level" is the right description here, though it may be clear to the reader anyway.
@felipecrv @jorisvandenbossche Thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lorentzenchr commented on pull request #37380: MINOR: [C++][Python] [Docs] clearer description of q argument of quantiles
Posted by "lorentzenchr (via GitHub)" <gi...@apache.org>.
lorentzenchr commented on PR #37380:
URL: https://github.com/apache/arrow/pull/37380#issuecomment-1719833499
Can I do anything else here to finish it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #37380: DOC clearer description of q argument of quantiles
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37380:
URL: https://github.com/apache/arrow/pull/37380#issuecomment-1693004869
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lorentzenchr commented on a diff in pull request #37380: MINOR: [C++][Python] [Docs] clearer description of q argument of quantiles
Posted by "lorentzenchr (via GitHub)" <gi...@apache.org>.
lorentzenchr commented on code in PR #37380:
URL: https://github.com/apache/arrow/pull/37380#discussion_r1311872692
##########
python/pyarrow/_compute.pyx:
##########
@@ -2145,7 +2145,8 @@ class QuantileOptions(_QuantileOptions):
Parameters
----------
q : double or sequence of double, default 0.5
- Quantiles to compute. All values must be in [0, 1].
+ Probability levels of the quantiles to compute. All values must be in
Review Comment:
Please see
- R (base!) [quantile](https://stat.ethz.ch/R-manual/R-devel/library/stats/html/quantile.html) argument probs
- https://github.com/numpy/numpy/pull/23529
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on pull request #37380: MINOR: [C++][Python] [Docs] clearer description of q argument of quantiles
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on PR #37380:
URL: https://github.com/apache/arrow/pull/37380#issuecomment-1720863443
Thanks @lorentzenchr!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lorentzenchr commented on a diff in pull request #37380: MINOR: [C++][Python] [Docs] clearer description of q argument of quantiles
Posted by "lorentzenchr (via GitHub)" <gi...@apache.org>.
lorentzenchr commented on code in PR #37380:
URL: https://github.com/apache/arrow/pull/37380#discussion_r1312172608
##########
python/pyarrow/_compute.pyx:
##########
@@ -2145,7 +2145,8 @@ class QuantileOptions(_QuantileOptions):
Parameters
----------
q : double or sequence of double, default 0.5
- Quantiles to compute. All values must be in [0, 1].
+ Probability levels of the quantiles to compute. All values must be in
Review Comment:
I have to disagree. We should really trust R here and learn from statisticians.
Let's say I want to calculate the 50%-quantile and the value is 33. Then 33 is the *quantile* while 50% is the corresponding *probability* or, more specific, probability level of the quantile. 33 and 50% are 2 different numbers for 2 different quantities with 2 different names.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #37380: MINOR: [C++][Python] [Docs] clearer description of q argument of quantiles
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #37380:
URL: https://github.com/apache/arrow/pull/37380#discussion_r1313272622
##########
python/pyarrow/_compute.pyx:
##########
@@ -2145,7 +2145,8 @@ class QuantileOptions(_QuantileOptions):
Parameters
----------
q : double or sequence of double, default 0.5
- Quantiles to compute. All values must be in [0, 1].
+ Probability levels of the quantiles to compute. All values must be in
Review Comment:
This change seems to make a lot of sense to you, so I won’t oppose it based on a weak sense that it doesn’t make the meaning less ambiguous.
##########
python/pyarrow/_compute.pyx:
##########
@@ -2145,7 +2145,8 @@ class QuantileOptions(_QuantileOptions):
Parameters
----------
q : double or sequence of double, default 0.5
- Quantiles to compute. All values must be in [0, 1].
+ Probability levels of the quantiles to compute. All values must be in
Review Comment:
This change seems to make a lot of sense to you, so I won’t oppose it based on a weak sense that it doesn’t make the meaning less ambiguous.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lorentzenchr commented on a diff in pull request #37380: MINOR: [C++][Python] [Docs] clearer description of q argument of quantiles
Posted by "lorentzenchr (via GitHub)" <gi...@apache.org>.
lorentzenchr commented on code in PR #37380:
URL: https://github.com/apache/arrow/pull/37380#discussion_r1312172608
##########
python/pyarrow/_compute.pyx:
##########
@@ -2145,7 +2145,8 @@ class QuantileOptions(_QuantileOptions):
Parameters
----------
q : double or sequence of double, default 0.5
- Quantiles to compute. All values must be in [0, 1].
+ Probability levels of the quantiles to compute. All values must be in
Review Comment:
I have to strongly disagree. We should really trust R here and learn from statisticians.
Let's say I want to calculate the 50%-quantile and the value is 33. Then 33 is the *quantile* while 50% is the corresponding *probability* or, more specific, probability level of the quantile. 33 and 50% are 2 different numbers for 2 different quantities with 2 different names.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on a diff in pull request #37380: MINOR: [C++][Python] [Docs] clearer description of q argument of quantiles
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #37380:
URL: https://github.com/apache/arrow/pull/37380#discussion_r1312775041
##########
python/pyarrow/_compute.pyx:
##########
@@ -2145,7 +2145,8 @@ class QuantileOptions(_QuantileOptions):
Parameters
----------
q : double or sequence of double, default 0.5
- Quantiles to compute. All values must be in [0, 1].
+ Probability levels of the quantiles to compute. All values must be in
Review Comment:
> The original t-Digest paper refers to these as quantiles [1]
The proposed change here also still calls the "thing" we want to compute quantiles ("Probability levels of the quantiles to compute"), so I don't think that's in contrast with how it is worded in the quoted text.
Now, this is out of my comfort zone, so personally I am happy to follow the lead of numpy to follow the terminology used in R and julia.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche merged pull request #37380: MINOR: [C++][Python] [Docs] clearer description of q argument of quantiles
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche merged PR #37380:
URL: https://github.com/apache/arrow/pull/37380
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org