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 for product of Dirichlet #322

Merged
merged 7 commits into from
Aug 14, 2024
Merged

Fix for product of Dirichlet #322

merged 7 commits into from
Aug 14, 2024

Conversation

torfjelde
Copy link
Member

logpdf_with_trans currently doesn't handle stuff like products of Dirichlet

Long-term we want to remove logpdf_with_trans completely, but for now this is holding up TuringLang/DynamicPPL.jl#575 so seems worth providing a hotfix.

@torfjelde
Copy link
Member Author

@yebai @sunxd3 @mhauru

src/Bijectors.jl Outdated Show resolved Hide resolved
@mhauru
Copy link
Member

mhauru commented Aug 7, 2024

@devmotion has a fair comment, but the code looks good to me otherwise. I'm confused by the CI failure, the new test passes for me locally.

Co-authored-by: David Widmann <[email protected]>
@torfjelde
Copy link
Member Author

torfjelde commented Aug 12, 2024

@devmotion good shout 👍

but the code looks good to me otherwise. I'm confused by the CI failure, the new test passes for me locally.

Seems to be a julia v1.6 thing. Looking at the deps used, I notice DistributionsAD is on 0.6.45 vs. 0.6.55 (on Julia latest). Miiight be it, though uncertain why. Lookin' into it.

EDIT: ah, it's just eachslice not supporting multiple dims on Julia v1.6 😕

@torfjelde
Copy link
Member Author

The way we use eachslice is only valid on Julia 1.9 and onwards 😕

@torfjelde torfjelde self-assigned this Aug 12, 2024
@mhauru
Copy link
Member

mhauru commented Aug 12, 2024

There's mapslices that has been taking multiple dimensions for longer than eachslice, I think switching to using that could work, since we call map on the results of eachslice. I'm happy to give this a go given @torfjelde, unless you're already in the middle of solving it.

@torfjelde
Copy link
Member Author

Go for it!

@mhauru
Copy link
Member

mhauru commented Aug 12, 2024

mapslices doesn't accept multiple arguments the way map does, so I ended up doing some reshaping instead: #323

* Work around eachslice limitation on Julia <v1.9

* Bug fix

* Fix Tapir tests
torfjelde and others added 2 commits August 13, 2024 22:29
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@mhauru mhauru merged commit 891c832 into master Aug 14, 2024
27 checks passed
@mhauru mhauru deleted the torfjelde/product-fix branch August 14, 2024 08:10
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.

4 participants