Skip to content

Add laplace_cdf and inverse_laplace_cdf functions#17838

Merged
highker merged 1 commit intoprestodb:masterfrom
jonhehir:laplace-fns
Jun 8, 2022
Merged

Add laplace_cdf and inverse_laplace_cdf functions#17838
highker merged 1 commit intoprestodb:masterfrom
jonhehir:laplace-fns

Conversation

@jonhehir
Copy link
Copy Markdown
Contributor

@jonhehir jonhehir commented Jun 3, 2022

Added CDF and inverse CDF functions corresponding to the Laplace distribution, commonly used in applications of differential privacy. These functions (and their documentation) mimic the form of the other distributions that have been implemented in Presto (e.g., normal, binomial, etc.).

Test plan - Tested by executing manual queries in presto-cli as well as through a small suite of added unit tests. Built docs render appropriately.

== RELEASE NOTES ==

General Changes
* Add :func:`laplace_cdf` and :func:`inverse_laplace_cdf` functions

@jonhehir jonhehir requested a review from a team as a code owner June 3, 2022 22:17
@jonhehir jonhehir requested a review from NikhilCollooru June 3, 2022 22:17
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented Jun 3, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: jonhehir / name: Jonathan Hehir (f55dcc6d5947e415b9f8a874f5653390426622f0)

@jonhehir jonhehir force-pushed the laplace-fns branch 2 times, most recently from 7d5f6ff to 39d34f4 Compare June 4, 2022 02:32
@ajaygeorge
Copy link
Copy Markdown
Contributor

@jonhehir For the Release Note section, functions need to be represented with :func:.
See https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines#formatting

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a constructor which takes in just mean and scale.
You can use this instead. new LaplaceDistribution(mean, scale);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! While that does look cleaner, passing the explicit null here is in line with how we handle the other distributions in MathFunctions. According to the docs, the LaplaceDistribution(double, double) constructor implicitly creates a new instance of an RNG, and they recommend explicitly passing a null to the LaplaceDistribution(RandomGenerator, double, double) constructor instead if no sampling is required (which is the case here).

Let me know if we should keep it this way or if you'd like me to change this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a constructor which takes in just mean and scale.
You can use this instead. new LaplaceDistribution(mean, scale);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

@jonhehir
Copy link
Copy Markdown
Contributor Author

jonhehir commented Jun 5, 2022

@jonhehir For the Release Note section, functions need to be represented with :func:. See https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines#formatting

Oops! Sorry about that. Edit: Should be fixed now!

@highker highker merged commit ac78220 into prestodb:master Jun 8, 2022
@highker highker mentioned this pull request Jul 6, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants