Adding the chi square distribution #15799

talgalili · 2021-03-10T14:14:05Z

Adding the chi square distribution, which is central to many statistical procedures (https://en.wikipedia.org/wiki/Chi-square_distribution) (#15798)

Test plan (adding unit-tests)

Following the diff template of: https://github.com/prestodb/presto/pull/11981/files

== RELEASE NOTES ==

General Changes
* Add chisquared_cdf() and inverse_chisquared_cdf() functions.

(like the beta_cds: https://prestodb.io/docs/current/release/release-0.215.html?highlight=beta_cds)

linux-foundation-easycla · 2021-03-10T14:14:09Z

The committers are authorized under a signed CLA.

✅ Tal Galili (32305650ec74d2803365aaeb2344f105aee3717e)

presto-docs/src/main/sphinx/functions/math.rst

talgalili · 2021-03-22T15:59:42Z

Thanks, good catch. I'll fix and resubmit later today (/or early tomorrow). Any other comments to address? T

…

On Mon, Mar 22, 2021, 17:49 Jinlin Zhang ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In presto-docs/src/main/sphinx/functions/math.rst <#15799 (comment)>: > @@ -85,6 +85,18 @@ Mathematical Functions The a, b parameters must be positive real numbers and value v must be a real value. The value v must lie on the interval [0, 1]. +.. function:: inverse_chisquared_cdf(df, p) -> double + + Compute the inverse of the Chi-square cdf with given df (degrees of freedom) parameter for the cumulative + probability (p): P(N < n). The the df parameter must be positive real values. Please remove the extra "the". — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15799 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHOJBVSDAHCTEXAS2EUFIDTE5RJ3ANCNFSM4Y6DLR6Q> .

talgalili · 2021-03-23T16:54:49Z

Hey @v-jizhang ,
tl;dr: I think this diff is ready for review / merging.

Details:
I've fixed the type in the doc, as well as found/fixed a bug in the testing. As far as I can see it all seems to work fine now. There is still some test that didn't finish running (but it doesn't seem related to this diff).
I'll re-base the diff, and have the tests re-run.

Cheers,
T

talgalili · 2021-03-23T17:29:54Z

(rebased)

v-jizhang

Looks good to me.

rongrong · 2021-03-26T18:14:59Z

presto-docs/src/main/sphinx/functions/math.rst

@@ -85,6 +85,18 @@ Mathematical Functions
    The a, b parameters must be positive real numbers and value v must be a real value.
    The value v must lie on the interval [0, 1].

+.. function:: inverse_chi_squared_cdf(df, p) -> double


Please put the function documentation in alphabetical order. The cdf documentation were in wrong places. You don't need to fix them in this PR, just make sure the new function documentations are added at the correct place.

Given that we intend to add more distributions (t, F, Gamma, etc.). I think a better approach is to have a dedicated section in the doc called "Probability distributions" (similar to how the doc currently has "Statistical Functions", "Trigonometric Functions", etc.)

I can do it in a separate diff before-or-after we finish adding all the distribution functions. Makes sense?

Makes sense. You can add a separate PR to refactor the documentation first, and rebase this PR on top of that one. Or if you prefer to do that afterwards, let's put these functions in the conventional order for this PR and modify it later.

presto-docs/src/main/sphinx/functions/math.rst

rongrong

Looks good. Please fix the nits. Thanks for contributing!

presto-main/src/main/java/com/facebook/presto/operator/scalar/MathFunctions.java

talgalili · 2021-03-26T20:32:33Z

Thanks Rongrong! I'll fix it early next week and will ping once it's done. T

…

On Fri, Mar 26, 2021, 21:19 Rongrong Zhong ***@***.***> wrote: ***@***.**** commented on this pull request. Looks good. Please fix the nits. Thanks for contributing! ------------------------------ In presto-main/src/main/java/com/facebook/presto/operator/scalar/MathFunctions.java <#15799 (comment)>: > @@ -704,6 +705,32 @@ public static double betaCdf( return distribution.cumulativeProbability(value); } + @description("inverse of ChiSquared cdf given df parameter and probability") + @ScalarFunction + @SqlType(StandardTypes.DOUBLE) + public static double inverseChiSquaredCdf( + @SqlType(StandardTypes.DOUBLE) double df, + @SqlType(StandardTypes.DOUBLE) double p) + { + checkCondition(p >= 0 && p <= 1, INVALID_FUNCTION_ARGUMENT, "p must be in the interval [0, 1]"); + checkCondition(df > 0, INVALID_FUNCTION_ARGUMENT, "df must be > 0"); must be greater than 0 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15799 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHOJBTDFXQ3EN2R5SLKR7TTFTF2ZANCNFSM4Y6DLR6Q> .

talgalili · 2021-03-28T06:52:37Z

@rongrong - thanks for the diff review.
I've addressed all the issues you've raised (except for the location of the help doc, please see my suggestion above).

Assuming it's all good, please move forward with merging this diff.
Once I see you gave this the green light, me and others from the hackathon will proceed to making all other "distribution function" go through the same workflow.

Thanks!

talgalili · 2021-04-09T16:55:56Z

Hey @rongrong :)
Any update on reviewing this diff? Can it be merged?

talgalili · 2021-04-14T18:21:10Z

Great. I'll modify this diff (in the coming two hours) to follow your proposed order, and will submit a new diff to create a new section in the doc in tge coming weeks. Will update soon. T

…

On Wed, Apr 14, 2021, 21:14 Rongrong Zhong ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In presto-docs/src/main/sphinx/functions/math.rst <#15799 (comment)>: > @@ -85,6 +85,18 @@ Mathematical Functions The a, b parameters must be positive real numbers and value v must be a real value. The value v must lie on the interval [0, 1]. +.. function:: inverse_chi_squared_cdf(df, p) -> double Makes sense. You can add a separate PR to refactor the documentation first, and rebase this PR on top of that one. Or if you prefer to do that afterwards, let's put these functions in the conventional order for this PR and modify it later. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15799 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHOJBVTGUNSWW5HGSIEM3DTIXLOVANCNFSM4Y6DLR6Q> .

talgalili · 2021-04-14T19:16:01Z

Hey @rongrong - I've now:

re-positioned the function documentation in the (hopefully) correct location
re-based and updated the PR

Please let me know if this now works.

Once Merged, I'll update the other contributors of all the learnings gained in this PR for me, and we'll proceed with preparing more diffs.

Yours,
T

…cal procedures (https://en.wikipedia.org/wiki/Chi-square_distribution) (prestodb#15798)

leepface mentioned this pull request Mar 11, 2021

Main task for statistical functions in presto hackathon #15798

Closed

6 tasks

talgalili mentioned this pull request Mar 16, 2021

Adding the chi square distribution (chisquared_cdf + inverse_chisquared_cdf) #15839

Closed

v-jizhang reviewed Mar 22, 2021

View reviewed changes

presto-docs/src/main/sphinx/functions/math.rst Outdated Show resolved Hide resolved

talgalili force-pushed the dist_chisquare branch 5 times, most recently from 8f0545c to cb10950 Compare March 23, 2021 06:27

talgalili force-pushed the dist_chisquare branch from cb10950 to be5b6a3 Compare March 23, 2021 17:28

v-jizhang approved these changes Mar 23, 2021

View reviewed changes

talgalili force-pushed the dist_chisquare branch from be5b6a3 to 46fd3df Compare March 25, 2021 09:24

rongrong reviewed Mar 26, 2021

View reviewed changes

presto-docs/src/main/sphinx/functions/math.rst Outdated Show resolved Hide resolved

rongrong reviewed Mar 26, 2021

View reviewed changes

presto-main/src/main/java/com/facebook/presto/operator/scalar/MathFunctions.java Outdated Show resolved Hide resolved

talgalili force-pushed the dist_chisquare branch from 46fd3df to eb3b130 Compare March 28, 2021 06:47

talgalili force-pushed the dist_chisquare branch 2 times, most recently from a3f0118 to cf73590 Compare April 14, 2021 19:12

rongrong approved these changes Apr 23, 2021

View reviewed changes

Adding the chi square distribution, which is central to many statisti…

d80da1c

…cal procedures (https://en.wikipedia.org/wiki/Chi-square_distribution) (prestodb#15798)

talgalili force-pushed the dist_chisquare branch from cf73590 to d80da1c Compare April 23, 2021 18:53

rongrong merged commit 6a1d1d7 into prestodb:master Apr 23, 2021

vaishnavibatni mentioned this pull request Apr 27, 2021

Add release notes for 0.252 #16013

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the chi square distribution #15799

Adding the chi square distribution #15799

talgalili commented Mar 10, 2021

linux-foundation-easycla bot commented Mar 10, 2021 •

edited

Loading

talgalili commented Mar 22, 2021 via email

talgalili commented Mar 23, 2021

talgalili commented Mar 23, 2021

v-jizhang left a comment

rongrong Mar 26, 2021

talgalili Mar 28, 2021 •

edited

Loading

rongrong Apr 14, 2021

rongrong left a comment

talgalili commented Mar 26, 2021 via email

talgalili commented Mar 28, 2021

talgalili commented Apr 9, 2021

talgalili commented Apr 14, 2021 via email

talgalili commented Apr 14, 2021

Adding the chi square distribution #15799

Adding the chi square distribution #15799

Conversation

talgalili commented Mar 10, 2021

linux-foundation-easycla bot commented Mar 10, 2021 • edited Loading

talgalili commented Mar 22, 2021 via email

talgalili commented Mar 23, 2021

talgalili commented Mar 23, 2021

v-jizhang left a comment

Choose a reason for hiding this comment

rongrong Mar 26, 2021

Choose a reason for hiding this comment

talgalili Mar 28, 2021 • edited Loading

Choose a reason for hiding this comment

rongrong Apr 14, 2021

Choose a reason for hiding this comment

rongrong left a comment

Choose a reason for hiding this comment

talgalili commented Mar 26, 2021 via email

talgalili commented Mar 28, 2021

talgalili commented Apr 9, 2021

talgalili commented Apr 14, 2021 via email

talgalili commented Apr 14, 2021

linux-foundation-easycla bot commented Mar 10, 2021 •

edited

Loading

talgalili Mar 28, 2021 •

edited

Loading