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

Fix inverse_cdf for Gamma #227

Merged
merged 5 commits into from
May 24, 2024
Merged

Fix inverse_cdf for Gamma #227

merged 5 commits into from
May 24, 2024

Conversation

YeungOnion
Copy link
Contributor

Don't think there's a closed form expression for inverting the CDF, so this improves on the bisection approach with a few iterations of Newton-Raphson since we have pdf

also adds tests

@YeungOnion YeungOnion linked an issue May 4, 2024 that may be closed by this pull request
@@ -146,6 +147,51 @@ impl ContinuousCDF<f64, f64> for Gamma {
gamma::gamma_ur(self.shape, x * self.rate)
}
}

fn inverse_cdf(&self, p: f64) -> f64 {
const MAX_ITERS: (u16, u16) = (8, 4);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a special reason for defining the number of iterations like this? It's probably more natural to just

for _ in 0..8 {

since the complexity is pretty low

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought,

8 and 4 sound like nice numbers, what does that give me. 8 bisection is 2 digits in $p$ and NR is quadratic convergence, so 4 gives 16 digits, lucky me. Hmm, it's helpful to name things, so const it is.

Then I thought,

I don't want to name two close but separate things, tuple it is.

I.e. I would be fine changing it to literal.

However, since my attention is on it, I think I might instead do the initial bracketing a little differently and try some math to back up the number of iterations I select.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. It's not a problem either way, I just wanted to know why you used that const tuple there.

@YeungOnion YeungOnion force-pushed the fix-inversecdf-gamma branch 2 times, most recently from 9041a98 to f4a85d1 Compare May 23, 2024 23:30
@YeungOnion YeungOnion force-pushed the fix-inversecdf-gamma branch from f4a85d1 to cc96883 Compare May 24, 2024 00:48
@YeungOnion
Copy link
Contributor Author

Since it works and I've not already reviewed how to make it "better" I don't think I will for now.

If we get a handle on provided or emitting precisions then I think property testing shrinking could use some of that precision information to help find larger parameter space with correct behavior.

@YeungOnion YeungOnion merged commit 6699215 into master May 24, 2024
2 checks passed
@YeungOnion YeungOnion deleted the fix-inversecdf-gamma branch May 25, 2024 00:14
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.

Inverse cdf inconsistency with cdf for the gamma distibution
2 participants