Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add LKJ bijector #125
Add LKJ bijector #125
Changes from 42 commits
a8c32a2
ba1ab40
a98ce04
74c902b
9c1742c
a1f8ec7
1c9b4ea
a52bdce
ead7e71
32cab00
1859672
1ef50b0
5ecaaee
bf41efc
52e74e4
be3dddf
476ca0f
bfd8d4f
dd0fade
38785f5
463acdc
5bdea37
131c502
4bbe656
8d2fc60
1120763
eec4de0
d9e0abf
1f24e65
09297e9
388fee0
72a16a8
d2908d5
8eb0686
9260c61
02a7d7e
d0984a4
10c2aed
fc6af4f
5ee5826
917605f
32497a1
c82e57c
c13d8a2
01250ce
de86456
5238efe
3a01d90
c86bcdf
79cbc5e
01db0f4
8cfc22d
9c9846b
9746499
7fb2675
bad759b
7710187
5228310
ea71cd7
a577b05
50b837d
b04252b
6c525a7
7e46a09
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Do we need a dense matrix? Seems a bit wasteful. I would have thought that
Array(cholesky(x).U)
orconvert(Array, cholesky(x).U)
would work as well.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 array
z
and the loops below and above are not needed, it seems you could just writeThere 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 can't we just return a triangular matrix so we don't have to fill the other elements?
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.
It is not needed by ForwardDiff, but it's not free for other 3 reversediff backends since it can be used to avoid an extra tanh call.
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.
There's a test to ensure input and output have same type:
Bijectors.jl/test/transform.jl
Line 82 in b0aaa98
So a zero filling is necessary, a triangular matrix just postpone the filling to another point.
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.
But it's the standard method here that is used for ForwardDiff, for reverse-mode you defined custom adjoints anyways. And even in the reverse-mode case it probably makes sense to move everything into one loop and fill both
z
andw
there to avoid all the unnecessary computations. Then you can even makez
aLowerTriangular
which would probably speed up the computations in the backward pass.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.
That seems weird but shouldn't be touched in this PR. @torfjelde Is that a general design choice?
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.
I tried again and rediscovery a forgotten problem. ReverseDiff doesn't support array with
Base.IndexStyle(::A) != Base.IndexLinear()
(see limit) butIndexStyle(LowerTriangular(randn(3,3))) == IndexCartesian()
. Passing aLowerTriangular
will throwAssertionError: IndexStyle(value) === IndexLinear()
error.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.
I mean you could still do it differently for ReverseDiff and just not wrap it in a
LowerTriangular
, I guess? My main point here was that there is no need to define the default method in a Zygote/ReverseDiff/Tracker compatible way since these AD backends will use the custom adjoints anyway. For the default method we should use the most efficient method. And even in the custom adjoints we can use the same optimized implementation logic and just keepz
(asLowerTriangular
or not, depending on the backend).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.
Ok I converted it to dense only in ReverseDiff. Anyway I still need to convert it to dense in last step to make that test happy.
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 suggestions above also apply to the custom adjoints.