Adding the Hypergeometric Distribution (hypergeometricCdf + inverseHypergeometricCdf) (#15798) #15821

sriram201 · 2021-03-12T01:08:54Z

Test plan - (Added unit tests)

== RELEASE NOTES ==

General Changes
* Add the hypergeometricCdf and inverseHypergeometricCdf functions.

linux-foundation-easycla · 2021-03-12T01:08:57Z

The committers are authorized under a signed CLA.

✅ sriram201 (d1f72af950b41b401ad4054176deb0686799e46f)

leepface · 2021-03-14T12:10:00Z

Code review:

squash down to one commit

Otherwise looks great!

v-jizhang

LGTM

talgalili · 2021-04-13T19:24:20Z

Hey @sriram201 ,
This diff looks good, but it seems to be missing the inverse_hypergeometric_cdf. Would you be able to please add it to this diff?

sriram201 · 2021-04-13T19:44:18Z

Hey @sriram201 ,
This diff looks good, but it seems to be missing the inverse_hypergeometric_cdf. Would you be able to please add it to this diff?

This seems to have been a pushing mixup. I fixed it

talgalili · 2021-04-13T19:50:38Z

Cool, thanks!

…

On Tue, Apr 13, 2021, 22:44 sriram201 ***@***.***> wrote: Hey @sriram201 <https://github.com/sriram201> , This diff looks good, but it seems to be missing the inverse_hypergeometric_cdf. Would you be able to please add it to this diff? This seems to have been a pushing mixup. I fixed it — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#15821 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHOJBRNFREU6VTXYCAZVVDTISNKDANCNFSM4ZBKP7UQ> .

talgalili · 2021-04-26T19:59:40Z

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

+.. function:: hypergeometric_cdf(populationSize, numberOfSuccesses, sampleSize, observedSuccesses) -> double
+
+    Compute the Hypergeometric cdf with given populationSize, numberOfSuccesses, sampleSize
+    and observed_successes:  P(N <= observedSuccesses). The populationSize must be a positive integer


Please change observedSuccesses to value (just to be consistent with the other functions)

(please explain in the text here that value is observedSuccesses)

talgalili · 2021-04-26T19:59:53Z

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

@@ -66,12 +66,25 @@ Mathematical Functions

    Returns the value of ``string`` interpreted as a base-``radix`` number.

+.. function:: hypergeometric_cdf(populationSize, numberOfSuccesses, sampleSize, observedSuccesses) -> double


Please change observedSuccesses to value (just to be consistent with the other functions)

(also in the function itself)

talgalili · 2021-04-26T20:01:05Z

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

+            @SqlType(StandardTypes.INTEGER) long populationSize,
+            @SqlType(StandardTypes.INTEGER) long numberOfSuccesses,
+            @SqlType(StandardTypes.INTEGER) long sampleSize,
+            @SqlType(StandardTypes.INTEGER) long observedSuccesses)


Please change observedSuccesses to value (just to be consistent with the other functions)

talgalili · 2021-04-26T20:01:32Z

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

+
+        HypergeometricDistribution distribution = new HypergeometricDistribution((int) populationSize, (int) numberOfSuccesses, (int) sampleSize);
+
+        return distribution.cumulativeProbability((int) observedSuccesses);


Please change observedSuccesses to value (just to be consistent with the other functions)

works for me

presto-main/src/test/java/com/facebook/presto/operator/scalar/TestMathFunctions.java

talgalili · 2021-04-26T20:12:00Z

Cool.

…

On Mon, Apr 26, 2021 at 11:10 PM sriram201 ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In presto-main/src/main/java/com/facebook/presto/operator/scalar/MathFunctions.java <#15821 (comment)>: > + @ScalarFunction + @SqlType(StandardTypes.DOUBLE) + public static double hypergeometricCdf( + @SqlType(StandardTypes.INTEGER) long populationSize, + @SqlType(StandardTypes.INTEGER) long numberOfSuccesses, + @SqlType(StandardTypes.INTEGER) long sampleSize, + @SqlType(StandardTypes.INTEGER) long observedSuccesses) + { + checkCondition(populationSize > 0, INVALID_FUNCTION_ARGUMENT, "populationSize must be greater than 0"); + checkCondition(numberOfSuccesses >= 0, INVALID_FUNCTION_ARGUMENT, "numberOfSuccesses cannot be negative"); + checkCondition(numberOfSuccesses <= populationSize, INVALID_FUNCTION_ARGUMENT, "numberOfSuccesses cannot exceed populationSize"); + checkCondition(sampleSize <= populationSize, INVALID_FUNCTION_ARGUMENT, "sampleSize cannot exceed populationSize"); + + HypergeometricDistribution distribution = new HypergeometricDistribution((int) populationSize, (int) numberOfSuccesses, (int) sampleSize); + + return distribution.cumulativeProbability((int) observedSuccesses); works for me — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#15821 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHOJBSAY2XDMWUVDL7R67TTKXCEHANCNFSM4ZBKP7UQ> .

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

talgalili · 2021-04-28T10:45:34Z

@rongrong the diffs seems updated and ready for review.

talgalili · 2021-05-10T19:16:55Z

Hey @sriram201 - I see that this diff has merge conflicts, could you please resolve them?

sriram201 · 2021-05-10T22:51:05Z

Hey @sriram201 - I see that this diff has merge conflicts, could you please resolve them?

Done. Sorry about that

Adding the CDF and the inverse CDF for the Hypergeometric Distribution

rohanpednekar · 2021-10-04T18:27:17Z

Gentle ping! 🙏

stale · 2022-04-16T14:11:33Z

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions!

leepface mentioned this pull request Mar 12, 2021

Main task for statistical functions in presto hackathon #15798

Closed

6 tasks

sriram201 force-pushed the hypergeometric_dist branch from b909dd1 to 18d8fb0 Compare March 14, 2021 17:57

sriram201 force-pushed the hypergeometric_dist branch from a06990d to db03528 Compare March 29, 2021 16:12

sriram201 mentioned this pull request Mar 29, 2021

Adding the hypergeometric distribution (hypergeometricCdf and inverseHypergeometriCdf) #15821 #15881

Closed

sriram201 force-pushed the hypergeometric_dist branch 3 times, most recently from 0fb078e to 515fdc4 Compare March 30, 2021 18:37

v-jizhang self-requested a review April 6, 2021 15:59

v-jizhang approved these changes Apr 6, 2021

View reviewed changes

sriram201 force-pushed the hypergeometric_dist branch 2 times, most recently from df36faf to da3c41d Compare April 13, 2021 22:02

sriram201 changed the title ~~Adding the CDF for the Hypergeometric Distribution (#15798)~~ Adding the Hypergeometric Distribution (hypergeometricCdf + inverseHypergeometricCdf) (#15798) Apr 26, 2021

sriram201 force-pushed the hypergeometric_dist branch from da3c41d to 997e4bd Compare April 26, 2021 19:11

talgalili reviewed Apr 26, 2021

View reviewed changes

presto-main/src/test/java/com/facebook/presto/operator/scalar/TestMathFunctions.java Show resolved Hide resolved

sriram201 force-pushed the hypergeometric_dist branch from 997e4bd to 8cca542 Compare April 26, 2021 20:19

rongrong reviewed Apr 27, 2021

View reviewed changes

sriram201 force-pushed the hypergeometric_dist branch from 8cca542 to ea10fae Compare April 27, 2021 20:40

sriram201 force-pushed the hypergeometric_dist branch from ea10fae to 678254b Compare May 10, 2021 22:49

Adding the Hypergeometric Distribution (prestodb#15798)

8c2fa52

Adding the CDF and the inverse CDF for the Hypergeometric Distribution

sriram201 force-pushed the hypergeometric_dist branch from 678254b to 8c2fa52 Compare May 24, 2021 23:48

stale bot added the stale label Apr 16, 2022

stale bot closed this Apr 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the Hypergeometric Distribution (hypergeometricCdf + inverseHypergeometricCdf) (#15798) #15821

Adding the Hypergeometric Distribution (hypergeometricCdf + inverseHypergeometricCdf) (#15798) #15821

sriram201 commented Mar 12, 2021 •

edited

Loading

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

edited

Loading

leepface commented Mar 14, 2021

v-jizhang left a comment

talgalili commented Apr 13, 2021

sriram201 commented Apr 13, 2021

talgalili commented Apr 13, 2021 via email

talgalili Apr 26, 2021

talgalili Apr 26, 2021

talgalili Apr 26, 2021

talgalili Apr 26, 2021

talgalili Apr 26, 2021

sriram201 Apr 26, 2021

talgalili commented Apr 26, 2021 via email

talgalili commented Apr 28, 2021

talgalili commented May 10, 2021

sriram201 commented May 10, 2021

rohanpednekar commented Oct 4, 2021

stale bot commented Apr 16, 2022

		@@ -66,12 +66,25 @@ Mathematical Functions

		Returns the value of ``string`` interpreted as a base-``radix`` number.

		.. function:: hypergeometric_cdf(populationSize, numberOfSuccesses, sampleSize, observedSuccesses) -> double


		HypergeometricDistribution distribution = new HypergeometricDistribution((int) populationSize, (int) numberOfSuccesses, (int) sampleSize);

		return distribution.cumulativeProbability((int) observedSuccesses);

Adding the Hypergeometric Distribution (hypergeometricCdf + inverseHypergeometricCdf) (#15798) #15821

Adding the Hypergeometric Distribution (hypergeometricCdf + inverseHypergeometricCdf) (#15798) #15821

Conversation

sriram201 commented Mar 12, 2021 • edited Loading

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

leepface commented Mar 14, 2021

v-jizhang left a comment

Choose a reason for hiding this comment

talgalili commented Apr 13, 2021

sriram201 commented Apr 13, 2021

talgalili commented Apr 13, 2021 via email

talgalili Apr 26, 2021

Choose a reason for hiding this comment

talgalili Apr 26, 2021

Choose a reason for hiding this comment

talgalili Apr 26, 2021

Choose a reason for hiding this comment

talgalili Apr 26, 2021

Choose a reason for hiding this comment

talgalili Apr 26, 2021

Choose a reason for hiding this comment

sriram201 Apr 26, 2021

Choose a reason for hiding this comment

talgalili commented Apr 26, 2021 via email

talgalili commented Apr 28, 2021

talgalili commented May 10, 2021

sriram201 commented May 10, 2021

rohanpednekar commented Oct 4, 2021

stale bot commented Apr 16, 2022

sriram201 commented Mar 12, 2021 •

edited

Loading

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

edited

Loading