Vector-version for PDBijector
#271
Conversation
and sizes for transformations
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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>
* 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]>
|
I made an issue to remind us that we should improve this implementation in the future. But for now I think we should just make it work, and then we can make it fast later. |
* Remove unused proj field * Update simplex bijector calls * Update simplex jacobian calls * Remove proj type entry * Compute logdetjac from square part of jacobian * Increment minor version number * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * Update test/interface.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fixed link and invlink for SimplexBijector * Update src/Bijectors.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * super-hacky fix to size issue of TransformedDistribution * added fixme comment * removed redundant constructor for Stacked * added implementation of output_size for SimplexBijector * Update src/bijectors/simplex.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fixed tests * removed more references to old SimplexBijector code * fixed more dirichlet tests * formatting * possilby fixed weird formatting complaints * 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]> Co-authored-by: Hong Ge <[email protected]> Co-authored-by: Tor Erlend Fjelde <[email protected]>
|
Just pushing this once more: the release of 0.13 is just waiting for this PR. We should really get this in the same batch because it will technically be breaking. Otherwise we'll have to release 0.14 immediately after 0.13 😕 Could someone maybe just give it a quick look-over and hit approve? 😬 There really isn't anything controversial in here + it just reuses existing implementation. |
|
Well shit. I messed up. My previous comment no longer holds true 😬 |
ext/BijectorsTrackerExt.jl
Outdated
| @@ -529,7 +529,27 @@ end | |||
|
|
|||
| # NOTE: Probably doesn't work in complete generality. | |||
| wrap_chainrules_output(x) = x | |||
| function wrap_chainrules_output(x::ChainRulesCore.Thunk) | |||
There was a problem hiding this comment.
Is this covered by the tests?
There was a problem hiding this comment.
No it isn't! But do we want to? 😕
I just started writing a test, and the realized it means I have to move wrap_chainrules__output to Bijectors.jl itself rather than as an extension. But it's only really used for Tracker (it is also used for one part in ReverseDiffjl, but this can be dropped in favor of the macro that has been added to ReverseDiff.jl).
There was a problem hiding this comment.
No it isn't!
But does that mean it's not needed?
There was a problem hiding this comment.
Ah, well, yes 🙃
Also, just realized, the reason why it's probably not there is because this will only be called from Tracker.@grad function which of course should never use @thunk 😬 I'll remove it 👍
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
Haha, I wanted to defer the release of Bijectors 0.13 until this had been merged, but in my excitement I forgot and made a release. Hence my comments regarding this above ain't no truth anymore 😬 But I think just releasing this as 0.13.1 is fine so np 👍 |
|
I believe comments have been addressed:) |
Similar vein to #246 and #263.
NOTE: This is based on #270 .