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

[Merged by Bors] - feat : Add Gamma distribution #9408

Closed
wants to merge 18 commits into from

Conversation

JADekker
Copy link
Collaborator

@JADekker JADekker commented Jan 3, 2024

Add pdf, CDF and measure of Gamma distribution.
Add proof that this is indeed a probability distribution. Add proofs that relate the various definitions.

TODO: Refactor Probability/Distributions/Exponential.lean using calls to
the results in this file.


Open in Gitpod

Copy link
Collaborator

@loefflerd loefflerd left a comment

Choose a reason for hiding this comment

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

I've left a bunch of comments. Most of these are just code-style fixes and "golf". The only comment of substance is that I think pow_exp_integral_Ioi should be proved in more generality (allowing complex exponents and deducing the real case from that) and moved to a different file.

Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
@JADekker
Copy link
Collaborator Author

JADekker commented Jan 3, 2024

I've left a bunch of comments. Most of these are just code-style fixes and "golf". The only comment of substance is that I think pow_exp_integral_Ioi should be proved in more generality (allowing complex exponents and deducing the real case from that) and moved to a different file.

Thank you, I'll take a good look tomorrow! Regarding some style comments: I took everything from Probability/Distributions/Exponential.lean and tried to stick as closely to that as possible; I should've probably double-checked style, I'll do that tomorrow!

@JADekker JADekker added awaiting-review awaiting-CI new-contributor This PR was made by a contributor with fewer than 5 merged PRs. Welcome to the community! labels Jan 4, 2024
@RemyDegenne RemyDegenne added the t-measure-probability Measure theory / Probability theory label Jan 4, 2024
Add pdf, CDF and measure of Gamma distribution.
Add proof that this is indeed a probability distribution.
Add proofs that relate the various definitions.

TODO: Refactor Probability/Distributions/Exponential.lean using calls to
  the results in this file.
Add pow_exp_integral_Ioi_complex, this extends pow_exp_integral_Ioi
to complex numbers. Should probably be moved to one of the files for
the Gamma functions.

Fix some style issues.
Move pow_exp_integral_Ioi_complex to SpecialFunctions/Gamma/Basic and
rename it. Once we agree on the definitive location,
pow_exp_integral_Ioi in Distributions/Gamma should be reduced to a call
to this result.
Rename pow_exp_integral_Ioi to integral_rpow_mul_exp_neg_mul_Ioi
Rename pow_exp_integral_Ioi_complex to integral_cpow_mul_exp_neg_mul_Ioi
Move integral_cpow_mul_exp_neg_mul_Ioi to SpecialFunctions/Gamma/Basic
Remove lintegral_Iic_eq_lintegral_Iio_add_Icc and
lintegral_eq_lintegral_Ici_add_Iio from Distributions/Exponential,
these are now imported through Distributions/Gamma, as Gamma should
precede Exponential after refactoring.

Important sorry: deriving integral_rpow_mul_exp_neg_mul_Ioi from
integral_cpow_mul_exp_neg_mul_Ioi is incomplete!
Move integral_rpow_mul_exp_neg_mul_Ioi to SpecialFunctions/Gamma/Basic
Remove duplicate theorem from Distributions/Gamma
refactor integral_rpow_mul_exp_neg_mul_Ioi using
integral_cpow_mul_exp_neg_mul_Ioi.
@loefflerd
Copy link
Collaborator

@collares, @JADekker What's going on here? I was in the middle of reviewing this branch, as specifically requested by Josha, and suddenly it's all been changed with a force-push, destroying an hour's work I just spent carefully streamlining the proofs...

@JADekker
Copy link
Collaborator Author

JADekker commented Jan 4, 2024

I'm very sorry, I was not aware that that would happen. Somehow I bogged up my git and asked for help from @collares to set back my branch; I wasn't aware of these consequences... Is there some way to fix this? Your version is the most up-to-date version, so I'm more than happy if you use that one and make a new PR with it/overwrite the current one entirely, if that is possible!

@loefflerd
Copy link
Collaborator

Don't worry; I manually copy-pasted the changes from my local git branch to somewhere else, downloaded the new git branch, and I'm now back on track. BTW, judging from the latest comments on zulip, it looks like I gave you misleading advice about instance parameters – sorry for that!

@JADekker
Copy link
Collaborator Author

JADekker commented Jan 4, 2024

No worries! I'm fighting with git on another branch right now, I'll make sure I don't touch this one!

@loefflerd
Copy link
Collaborator

I've pushed a commit which shortens the proofs in Gamma/Basic.lean a good deal, and also makes them quicker to compile (since there are fewer repetitions of heavyweight tactics like simp and norm_cast). I'll look at the other file next.

Copy link
Collaborator

@loefflerd loefflerd left a comment

Choose a reason for hiding this comment

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

Here are some comment. I've tried to fix the spelling / grammar errors in the docstrings for you, since they aren't your fault. (Just in case you aren't aware: you can apply all the suggestions I've made as a batch, by going to the "Files changed" view – that hopefully makes dealing with pedantic reviewers slightly less tedious 😄)

Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Co-authored-by: David Loeffler <d.loeffler.01@cantab.net>
refactor lintegral_gammaPdf_eq_one, removes
pow_exp_integral_Ioi_insert_1 as this is superfluous now.
minor style changes.
@JADekker
Copy link
Collaborator Author

JADekker commented Jan 4, 2024

Thank you for all the fixes, I'll try to be more critical of spelling and grammar in the future as well! I forgot about the batch apply, thank you for reminding me!

@JADekker
Copy link
Collaborator Author

JADekker commented Jan 4, 2024

@loefflerd would you like me to start refactoring Probability/Distributions/Exponential.lean and tag that along to this PR, or wait until this PR has merged?

I think I'd also like to add the chi-squared distribution as a special case of the Gamma distribution, as it opens up the way for things like t-distributions; if there is no need for that, I'm happy to wait though!

@loefflerd
Copy link
Collaborator

I did some golfing on lintegral_gammaPdf_eq_one. This means lintegral_eq_lintegral_Ici_add_Iio is no longer required in this file, so I moved it back to its original home (temporarily; we can get rid of it entirely once that file gets refactored).

I'd advise waiting for a little while (until this gets merged) until you start work on the next stage. Since I am not a maintainer myself, somebody else will have to give the final OK to this, and they might ask for yet more changes; and it's always a pain to forward-port changes between multiple PR's in a series.

@loefflerd
Copy link
Collaborator

OK, this looks good to me now!

I suggest posting on the zulip "PR reviews" queue, asking for a maintainer to make the final merge decision.

@JADekker
Copy link
Collaborator Author

JADekker commented Jan 4, 2024

I’ll wait until it builds and then post it there! Thank you! Feel free to add yourself to the copyright statement of course, if you like!

Copy link
Contributor

@RemyDegenne RemyDegenne left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!
Here are 3 very minor comments. You can merge yourself after resolving them.

bors d+

Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Distributions/Gamma.lean Outdated Show resolved Hide resolved
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Jan 5, 2024

✌️ JADekker can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

Co-authored-by: Rémy Degenne <remydegenne@gmail.com>
@JADekker
Copy link
Collaborator Author

JADekker commented Jan 5, 2024

bors r+

mathlib-bors bot pushed a commit that referenced this pull request Jan 5, 2024
Add pdf, CDF and measure of Gamma distribution.
Add proof that this is indeed a probability distribution. Add proofs that relate the various definitions.

TODO: Refactor Probability/Distributions/Exponential.lean using calls to
  the results in this file.




Co-authored-by: David Loeffler <d.loeffler.01@cantab.net>
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Jan 5, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title feat : Add Gamma distribution [Merged by Bors] - feat : Add Gamma distribution Jan 5, 2024
@mathlib-bors mathlib-bors bot closed this Jan 5, 2024
@mathlib-bors mathlib-bors bot deleted the JADekker_gamma_distribution branch January 5, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated new-contributor This PR was made by a contributor with fewer than 5 merged PRs. Welcome to the community! t-measure-probability Measure theory / Probability theory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants