Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add weibull_cdf and inverse_weibull_cdf functions #15820

Merged
merged 1 commit into from
Jul 20, 2021
Merged

Add weibull_cdf and inverse_weibull_cdf functions #15820

merged 1 commit into from
Jul 20, 2021

Conversation

jsawruk
Copy link
Contributor

@jsawruk jsawruk commented Mar 12, 2021 •
edited
Loading

https://en.wikipedia.org/wiki/Weibull_distribution

Test plan - ./mvnw clean install -Dtest=TestMathFunctions -fn -pl presto-main 

== RELEASE NOTES ==

General Changes
* Add Weibull distribution CDF and inverse CDF functions to MathFunctions.java

@leepface leepface mentioned this pull request Mar 12, 2021
Closed
6 tasks
@leepface
Copy link
Contributor

Code review:

  • Looks like you have a bunch of unrelated changes. You need to rebase.
  • Tests are failing (might resolve after rebase)

@jsawruk jsawruk force-pushed the weibull_dist branch 2 times, most recently from 360f44b to 3700e20 Compare March 18, 2021 18:06
@jsawruk jsawruk mentioned this pull request May 7, 2021
Open
@jsawruk jsawruk force-pushed the weibull_dist branch 2 times, most recently from f15aafc to f7b874c Compare June 15, 2021 20:52
rongrong
rongrong approved these changes Jun 15, 2021
View reviewed changes
Copy link
Contributor

@rongrong rongrong left a comment

Choose a reason for hiding this comment

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

Please update the commit title to Add Weibull distribution functions

presto-docs/src/main/sphinx/functions/math.rst Outdated
@@ -99,6 +99,12 @@ Mathematical Functions
The lambda parameter must be a positive real number (of type DOUBLE).
The probability p must lie on the interval [0, 1).

.. function:: inverse_weibull_cdf(a, b, p) -> double

Compute the inverse of the Weibull cdf with given parameters a, b for the probability p.
Copy link
Contributor

Choose a reason for hiding this comment

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

I won't be too picky, but either back-quote all variables, or none. You are doing it for the weibull_cdf but not here. Also, no need to have a new line for every sentence.

@jsawruk jsawruk changed the title Added Weibull distribution (#15798) Add Weibull distribution Jun 15, 2021
@rongrong
Copy link
Contributor

Please change the commit title to Add Weibull distribution functions

@jsawruk jsawruk changed the title Add Weibull distribution Add weibull_cdf and inverse_weibull_cdf functions Jul 12, 2021
@rongrong rongrong merged commit 5dd94fe into prestodb:master Jul 20, 2021
@NikhilCollooru NikhilCollooru mentioned this pull request Aug 3, 2021
Merged
@vaishnavibatni vaishnavibatni mentioned this pull request Aug 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rongrong rongrong rongrong approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jsawruk @leepface @rongrong