-
Notifications
You must be signed in to change notification settings - Fork 34
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
Make SimplexBijector actually bijective #263
Conversation
Currently tests fails due to these lines, which seem to assume inputs and outputs are the same size
On Slack, @torfjelde confirmed that these should be fixed. |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/bijectors/simplex.jl
Outdated
K = length(x) | ||
@assert K > 1 "x needs to be of length greater than 1" | ||
dydxt = similar(x, length(x), length(x)) | ||
dydxt = similar(x, K, K - 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe merge it with the next line while you're at it to make it clearer that dydx
is initialized with zeros (I had missed that initially when reading the code)?
dydxt = similar(x, K, K - 1) | |
dydxt = fill!(similar(x, K, K - 1), 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(BTW the similar
+ AbstractVector
argument wrongly suggests that the code will work for arbitrary inputs - but the loops below assume that everything uses 1-based indices)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but this should be fixed in a separate PR.
end | ||
end | ||
return UpperTriangular(dydxt)' | ||
return dydxt' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The adjoint operation seems a bit annoying but I guess the algorithm should be updated in separate PRs if desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, agreed, but this should be fixed separately.
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Haven't forgotten about this, but the DPPL integration was set back significantly by some other changes we had made. Should be done soon now 👍 |
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>
Aaaalrighty! Does someone want to give this a look-over? I think pasts will pass now, an so it would be nice to get this merged. |
Thanks @torfjelde for the fixes! All LGTM, but someone else needs to review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just a minor question.
It's a breaking change it seems, so IMO it would be good to include the correct version bump in the PR to avoid accidentally tagging a non-breaking release.
We haven't released |
Co-authored-by: David Widmann <[email protected]>
* added output_length and output_size to compute output, well, leengths and sizes for transformations * added tests for size of transformed dist using VcCorrBijector * use already constructed transfrormation * TransformedDistribution should now also have correct variate form * added proper variateform handling for VecCholeskyBijector too * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * added output_size impl for Reshape too * bump minor version * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * Update src/interface.jl * Update src/bijectors/corr.jl * reverted removal of length as we'll need it now * updated Stacked to be compat with changing sizes * forgot to commit deetion * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * add testing of sizes to `test_bijector` * some more tests for stacked * Update test/bijectors/stacked.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * added awful generated function to determine output ranges for Stacked with tuple because recursive implementation fail * added slightly more informative comment * format * more fixes to that damned Stacked * Update test/interface.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * specialized constructors for Stacked further * fixed bug in output_size for CorrVecBijector * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: David Widmann <[email protected]>
Similar to #228, currently the
SimplexBijector
makes transformed distributions improper. A demo from slack:This PR changes
SimplexBijector
to transform aK
-vector to aK-1
-vector. Since theproj
type entry inSimplexBijector
only impacted the extraK
th entry of the unconstrained vector, this type entry has been removed. Since the Jacobian is now non-square, triangular return types are no longer used. As a result, the change is marked as breaking.