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.

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

Specialize inverse_cdf for LogNormal distribution #127

Closed

Conversation

Twister915
Copy link
Contributor

@Twister915 Twister915 commented Jan 22, 2021

Description

Specialize inverse_cdf for LogNormal distribution, because an equation exists and it is faster & more accurate than the default binary search implementation.

Changes

  • Adds a definition for inverse_cdf in ContinuousCDF impl for LogNormal distribution, according to the definition I read here and some pen & paper verification of this definition.
  • Generalizes the unit tests in LogNormal such that the CDF test cases are re-used (where applicable) to verify the inverse CDF definition

Review Notes

  • I'm no mathematician so someone definitely needs to verify my implementation. I am not sure if the wolfram-alpha I linked above is the most general definition of the inverse CDF.
    • Wikipedia (seemingly) has a different definition which includes erf_inverse, and I don't know how erf_inverse relates to erfc_inverse.
    • I inverted the CDF definition myself (using my basic algebra skills) on pen & paper and it matched the wolfram-alpha math, so that's what I implemented.
  • The unit testing section will perform the exact same test cases for the CDF, despite all the code changes. There is one exception: a change to the accuracy argument on the case where input=0.5 and output=0.0000000015011556178148777579869633555518882664666520593658.

@vks vks mentioned this pull request May 15, 2021
7 tasks
@troublescooter troublescooter self-requested a review May 16, 2021 13:29
@henryjac
Copy link
Contributor

henryjac commented Feb 29, 2024

erfc is the complementary error function, so erfc(x) = 1 - erf(x). Wikipedia and wolfram alpha give the same math as your code, so that checks out. Will check the tests soon.

I like the way the tests are done so we check the cdf against the inverse_cdf, could highlight problems such as 200 more clearly.

@henryjac henryjac added this to the 0.17 milestone Mar 6, 2024
@YeungOnion
Copy link
Contributor

merge edits

  • added } since added functions within impl ContinuousCDF for LogNormal were adjacent
  • math in docs should be text instead of ignore with b8a0ec2

I too like the symmetric tests.

@henryjac iirc you wanted to rebase and merge so merge commits would represent PRs in history. Would you prefer that @Twister915 redo the changes atop master so we can rebase instead and keep that up?

@YeungOnion
Copy link
Contributor

Opted to do a local merge and push to master, will have to close this one manually.

If anyone knows a way to manually modify this PR to "close as merged" by a specific commit, the SHA is ae5431f

Thanks for implementing this along with a thorough write-up!

@YeungOnion YeungOnion closed this May 24, 2024
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.

None yet

3 participants