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

Improvements to Composed and TruncatedBijector fixing #40 #52

Merged

Conversation

torfjelde
Copy link
Member

Overview

This PR introduces most of the performance improvements to Composed discussed in #46 and the TruncatedBijector discussed in #40 making bijector(d::Distribution) type-stable and providing a good "default" bijector.

In addition to the above, Test.@inferred is now used extensively in test/interface.jl to try and catch instability for the <:Tuple versions of Composed and Stacked.

Finally the project structure has been changed a bit (for the better). Implementations of Bijector are now found in the src/bijectors directory, e.g. src/bijectors/scale.jl. This makes it much easier for people unfamiliar with the code to contribute, plus easier to reason about in general.

Changes

Composed

From a user-perspective, the major change is that will now "flatten" the composition structure in the sense that

julia> using Bijectors; using Bijectors: Exp

julia> b = Exp()
Exp{0}()

julia> cb1 = b  b;

julia> cb2 = b  b;

julia> (cb1  cb2).ts # <= different
(Exp{0}(), Exp{0}(), Exp{0}(), Exp{0}())

julia> (cb1  cb2).ts isa NTuple{4, Exp{0}}
true

while in master it looks like

julia> using Bijectors; using Bijectors: Exp
[ Info: Recompiling stale cache file /home/tor/.julia/compiled/v1.1/Bijectors/39uFz.ji for Bijectors [76274a88-744f-5084-9051-94815aaf08c4]

julia> b = Exp()
Exp{0}()

julia> cb1 = b  b;

julia> cb2 = b  b;

julia> (cb1  cb2).ts # <= different
(Composed{Tuple{Exp{0},Exp{0}},0}((Exp{0}(), Exp{0}())), Composed{Tuple{Exp{0},Exp{0}},0}((Exp{0}(), Exp{0}())))

julia> (cb1  cb2).ts isa Tuple{Composed, Composed}
true

Note that this is only the case for . In some cases it might be desirable to keep certain operations together, e.g. Composed{Tuple{Scale, Shift}}} represents an affine transformation. Therefore composel and composer will not do this "flattening", but instead preserve the structure of the compositions.

The above is also described in the docstring of Composed.

src/bijectors/adbijector.jl Outdated Show resolved Hide resolved
src/bijectors/adbijector.jl Outdated Show resolved Hide resolved
src/bijectors/scale.jl Show resolved Hide resolved
src/bijectors/truncated.jl Show resolved Hide resolved
@xukai92
Copy link
Member

xukai92 commented Nov 17, 2019

Looks good to me overall!

@xukai92
Copy link
Member

xukai92 commented Nov 26, 2019

@torfjelde Can I merge this now?

@torfjelde
Copy link
Member Author

torfjelde commented Nov 27, 2019

@torfjelde Can I merge this now?

Yup! Though maybe I should bump the version number? I guess this would technically be a breaking change since the circ-operation now has different behavior. Should I bump the minor- or patch-version?

EDIT: Actually, let me do a "review" myself tomorrow and make sure everything is good. Shouldn't be any bugs, just filling in some simply additions e.g. just realized I had forgotten to implement the transform and logabsdetjac for Composed using the @generated approach we're currently doing for forward.

src/bijectors/composed.jl Outdated Show resolved Hide resolved
@torfjelde
Copy link
Member Author

Should be good now @kai!:)

@torfjelde torfjelde closed this Nov 27, 2019
@torfjelde torfjelde reopened this Nov 27, 2019
@xukai92 xukai92 merged commit dc736c8 into TuringLang:master Nov 28, 2019
@xukai92
Copy link
Member

xukai92 commented Nov 28, 2019

Thanks for the great work!

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.

None yet

2 participants