Adding the Poisson distribution by talgalili · Pull Request #15814 · prestodb/presto

talgalili · 2021-03-11T12:55:29Z

Adding the Poisson distribution, which is central to many statistical procedures (https://en.wikipedia.org/wiki/poisson_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 poisson_cdf and inverse_poisson_cdf functions.

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

leepface

Believe these are the maven-checks errors you're seeing. You may also need to rebase for the other errors that look unrelated.

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

rongrong

Looks good, just some nits. Please fix the checkstyle error.

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

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

talgalili · 2021-04-28T15:13:40Z

A point to notice:
The poisson distribution has an infinite support on the right hand side. Because the function returns an integer, it would not return an Inf object, but rather some MAXINT style number (a very large integer).
One option was to put that large number in the test. But instead, what we decided to do (Eden and me), is to just forbid the use of p=1. The reason is because that's also the current default that is used in inverse_normal_cdf, which has inf support, and it just forbids the use of p=1 (or p=0).
Also, in this recent update to the diff, I've changed inverse_poisson_cdf to return BIGINT instead of DOUBLE.

A followup to this discussion is that I should also change inverse_chisquare_cdf to not allow p=1 (right now it allows it, and returns Inf as expected, but that cases an inconsistent behavior between it and inverse_normal_cdf)

talgalili · 2021-04-28T16:07:33Z

(I may wish to move this to int from bigint, need to verify further. https://commons.apache.org/proper/commons-math/javadocs/api-3.5/org/apache/commons/math3/distribution/IntegerDistribution.html )

rongrong · 2021-04-28T19:45:01Z

Both checkstyle error and test failures are related. Please fix

talgalili · 2021-04-28T19:49:00Z

Will do, thanks.

…

On Wed, Apr 28, 2021, 22:45 Rongrong Zhong ***@***.***> wrote: Both checkstyle error and test failures are related. Please fix — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15814 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHOJBVCEF2IM57IVLX4FRTTLBQVBANCNFSM4ZAHIVSA> .

talgalili · 2021-04-29T11:56:41Z

@rongrong - diff was fixed, and all tests now came back green. It's back to you for review :)

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

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

Thanks @rongrong , I've now updated the text in the diff. I've also expanded a bit the description of inverse_poisson_cdf so it's clearer which value it return.

talgalili · 2021-05-09T07:15:28Z

Hey @rongrong - this diff includes all the fixes we've discussed. Could you please review and let me know if it's good to merge, or if there are other steps to take?
Thanks :)

rongrong

One last nits. Thanks!

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

talgalili · 2021-05-11T05:02:40Z

Thanks @rongrong - made the fixes, diff is now ready for merging :)

mbasmanova · 2021-05-13T14:09:37Z

Thank you, @talgalili, for the contribution.

talgalili · 2021-05-13T14:18:04Z

Thanks @mbasmanova for the help in merging this. :)
And @rongrong - for a great review.

kaikalur · 2021-05-13T14:45:47Z

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

+            @SqlType(StandardTypes.INTEGER) long value)
+    {
+        checkCondition(value >= 0, INVALID_FUNCTION_ARGUMENT, "value must be a non-negative integer");
+        checkCondition(lambda > 0, INVALID_FUNCTION_ARGUMENT, "lambda must be greater than 0");


Since the library API is already doing this check, I say just do a try/catch and throw user_error

Interesting. Since this is the method used by all the distribution functions (i.e.: normal, beta, chi-square, binomial), do you think it should be changed there as well?
If so - could you please help with this change? (I'm not experienced in Java, so want to make sure I understand what you're proposing and which error will be thrown)

Just do something like:

try { ... } catch(NotStrictlyPositiveException notStrictlyPositiveException) { throw new PrestoException(GENERIC_USER_ERROR, ...)

Look at StandardErrorCodes.java and other files to see the pattern what they do.

And yeah it will be good to do for all of them.

kaikalur · 2021-05-13T14:48:36Z

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

+    {
+        checkCondition(value >= 0, INVALID_FUNCTION_ARGUMENT, "value must be a non-negative integer");
+        checkCondition(lambda > 0, INVALID_FUNCTION_ARGUMENT, "lambda must be greater than 0");
+        PoissonDistribution distribution = new PoissonDistribution(lambda);


Is the lambda going to be generally fixed in a query? If so, you should find a way to avoid new object creation to improve memory perf.

Also interesting. As I wrote to your other comment - this is the method used by all the distribution functions (i.e.: normal, beta, chi-square, binomial), do you think it should be changed there as well?
If so - could you please help with this change / propose how to do it?

Thanks upfront.

I don't have a good suggestion here :( I looked in the code but can't find other examples. I will look further and comment here if I find something.

leepface mentioned this pull request Mar 11, 2021

Main task for statistical functions in presto hackathon #15798

Closed

6 tasks

leepface reviewed Mar 11, 2021

View reviewed changes

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

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

talgalili force-pushed the dist_poisson branch 7 times, most recently from 4a26de4 to 9213486 Compare April 27, 2021 12:20

rongrong reviewed Apr 27, 2021

View reviewed changes

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

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

talgalili force-pushed the dist_poisson branch 2 times, most recently from 0ab7b2a to 3311a86 Compare April 28, 2021 15:06

talgalili force-pushed the dist_poisson branch 5 times, most recently from 9b5c9c9 to c1f94d9 Compare April 29, 2021 10:49

rongrong reviewed Apr 29, 2021

View reviewed changes

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

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

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

talgalili force-pushed the dist_poisson branch 2 times, most recently from 929026e to 01c3bac Compare April 29, 2021 19:49

talgalili force-pushed the dist_poisson branch from 01c3bac to 48d0c36 Compare May 9, 2021 07:21

talgalili requested a review from rongrong May 10, 2021 19:17

rongrong approved these changes May 11, 2021

View reviewed changes

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

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

talgalili force-pushed the dist_poisson branch from 48d0c36 to ad86550 Compare May 11, 2021 05:01

talgalili requested a review from rongrong May 11, 2021 05:02

talgalili force-pushed the dist_poisson branch from ad86550 to 6138712 Compare May 13, 2021 04:51

Adding the Poisson distribution

c2c186e

talgalili force-pushed the dist_poisson branch from 6138712 to c2c186e Compare May 13, 2021 04:54

mbasmanova merged commit 39c66e3 into prestodb:master May 13, 2021

kaikalur reviewed May 13, 2021

View reviewed changes

sujay-jain mentioned this pull request May 21, 2021

Add release notes for 0.254 #16141

Merged

10 tasks

ajaygeorge mentioned this pull request May 26, 2021

[TEST] Add release notes for 0.254 #16165

Closed

16 tasks

Conversation

talgalili commented Mar 11, 2021 • edited by mbasmanova Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leepface left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rongrong left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

talgalili commented Apr 28, 2021

Uh oh!

talgalili commented Apr 28, 2021

Uh oh!

rongrong commented Apr 28, 2021

Uh oh!

talgalili commented Apr 28, 2021 via email

Uh oh!

talgalili commented Apr 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

talgalili commented Apr 29, 2021

Uh oh!

talgalili commented May 9, 2021

Uh oh!

rongrong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

talgalili commented May 11, 2021

Uh oh!

mbasmanova commented May 13, 2021

Uh oh!

talgalili commented May 13, 2021

Uh oh!

kaikalur May 13, 2021

Choose a reason for hiding this comment

Uh oh!

talgalili May 13, 2021

Choose a reason for hiding this comment

Uh oh!

kaikalur May 13, 2021

Choose a reason for hiding this comment

Uh oh!

kaikalur May 13, 2021

Choose a reason for hiding this comment

Uh oh!

kaikalur May 13, 2021

Choose a reason for hiding this comment

Uh oh!

talgalili May 13, 2021

Choose a reason for hiding this comment

Uh oh!

kaikalur May 13, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

talgalili commented Mar 11, 2021 •

edited by mbasmanova

Loading

rongrong left a comment •

edited

Loading