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

Make Gamma distribution multivariate #358

Merged
merged 6 commits into from
Feb 7, 2024
Merged

Make Gamma distribution multivariate #358

merged 6 commits into from
Feb 7, 2024

Conversation

nabriis
Copy link
Collaborator

@nabriis nabriis commented Feb 2, 2024

Closes #330 #331

@nabriis nabriis linked an issue Feb 2, 2024 that may be closed by this pull request
Multivariate Gamma returns a single value #331
Closed
jakobsj
jakobsj requested changes Feb 6, 2024
View reviewed changes
Copy link
Contributor

@jakobsj jakobsj left a comment

Choose a reason for hiding this comment

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

Thank you for adding this, it seems fairly straightforward so not many comments. I note from the error message that this PR stops short of allowing multivariate Gamma with the conjugate sampler. How big a job would that be?

cuqi/distribution/_gamma.py Outdated Show resolved Hide resolved
cuqi/distribution/_gamma.py Outdated Show resolved Hide resolved
cuqi/distribution/_gamma.py Show resolved Hide resolved
amal-ghamdi
amal-ghamdi requested changes Feb 6, 2024
View reviewed changes
Copy link
Contributor

@amal-ghamdi amal-ghamdi left a comment

Choose a reason for hiding this comment

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

Thank you @nabriis for this fix! I have only 3 comments for your consideration.

cuqi/distribution/_gamma.py Outdated Show resolved Hide resolved
cuqi/distribution/_gamma.py Show resolved Hide resolved
cuqi/distribution/_gamma.py Show resolved Hide resolved
amal-ghamdi
amal-ghamdi approved these changes Feb 6, 2024
View reviewed changes
Copy link
Contributor

@amal-ghamdi amal-ghamdi left a comment

Choose a reason for hiding this comment

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

Thank you for the update. LGTM!

@nabriis
Copy link
Collaborator Author

nabriis commented Feb 6, 2024

Thank you for adding this, it seems fairly straightforward so not many comments. I note from the error message that this PR stops short of allowing multivariate Gamma with the conjugate sampler. How big a job would that be?

Unsure how much effort it would be to support multivariate in conjugate. Likely just need to extend the equations to multivariate case, where now they assume scalar.

jakobsj
jakobsj approved these changes Feb 7, 2024
View reviewed changes
Copy link
Contributor

@jakobsj jakobsj left a comment

Choose a reason for hiding this comment

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

OK. Please park the conjugacy point as a new issue, refer to it here, consider the small suggestion to clarify the dimension in the example, and then go ahead and merge.

cuqi/distribution/_gamma.py Show resolved Hide resolved
@nabriis nabriis mentioned this pull request Feb 7, 2024
Open
@nabriis nabriis merged commit bacee57 into main Feb 7, 2024
6 checks passed
@nabriis nabriis deleted the make_gamma_dist_multivariate branch February 7, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amal-ghamdi amal-ghamdi amal-ghamdi approved these changes

@jakobsj jakobsj jakobsj 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.

Multivariate Gamma returns a single value Gamma distribution multivariate
3 participants
@nabriis @amal-ghamdi @jakobsj