TuringLang / Bijectors.jl

Implementation of normalising flows and constrained random variable transformations
https://turinglang.org/Bijectors.jl/
MIT License
199 stars 32 forks source link

Adding support for FunctionChains.jl? #222

Closed oschulz closed 1 year ago

oschulz commented 2 years ago

~In the spirit of #199, which resulted in InverseFunctions and ChangesOfVariables: I've been looking for a lightweight package that defines composed functions that go beyond Base.ComposedFunction, so that allow for arrays/iterators/tuples of functions, type-stable and performant. I couldn't really find anything lightweight that provides this (, though obviously the ML packages all have their own equivalent, and Bijectors has Composed.~

Update: There's FunctionChains.jl now.

(@giordano, did I overlook something?)

@willtebbutt, @devmotion, @torfjelde, @cscherrer would there be interest in creating such a common lightweight package like that? I'd volunteer to draft it.

cscherrer commented 2 years ago

Sounds good! I've often hoped for a common interface for this sort of thing, especially the currently-incompatible diffeomorphism implementations that are spread across a few packages like this one.

One thing I think it's important to account for is that high-dimensional transforms are themselves often implemented in terms of iteration. So we need ways to compose these iterations "vertically" and "horizontally". I've mostly worked with TransformVariables; I'm not sure how similar or different Bijectors approach might be

oschulz commented 2 years ago

I've registered FunctionChains.jl. It should, in principle, be able to replace Bijectors.Composed, though the result will obviously not be a subtype of Bijector (is that important, though?).

oschulz commented 1 year ago

I could add support for FunctionChains to Bijectors, if there's interest, e.g. via bi-directioinal conversion in a package extension?

devmotion commented 1 year ago

More a general question, not an actual objection at this point, but similar to the discussion in https://github.com/JuliaDiff/AbstractDifferentiation.jl/issues/88 it's unclear to me if bi-directional conversions should be defined here or if only Base.convert(::Type{T}, ...) for types that Bijectors owns should be defined in a possible extension of it.

oschulz commented 1 year ago

bi-directional conversions should be defined here or if only Base.convert(::Type{T}, ...) for types that Bijectors owns should be defined in a possible extension of it.

I agree, it would be nice to have a kind of convention here, to avoid "extension piracy". On the other hand, maintenance wise it would be much easier (versioning, CI, etc.) to not split and extension that "bridges" two package in respect to a specific topic, but to host the complete extension in one of them. Ideally the package maintainers would come to some agreement where to put the extensions in cases where the choice is not obvious.

If the two packages is very lightweight/abstract, I think would would technically better to host the extension in the "heavy" package. That way, one can use a non-weak depencency instead of Requires on Julia <v1.9, which will improve load times there. In this specific case here, for example, FunctionChains can be very lightweight (I'll move most of it's dependencies into extensions, and Bijectors already shares pretty much all of them already anyway). So Bijectors depending on FunctionChains on Julia <= v1.8 and using and extension on >= v1.9 seems better, load-time wise, than FunctionChains using Requires on Julia <= v1.8 (since Bijectors is much heavier) and using an extension on >= v1.9.

torfjelde commented 1 year ago

I could add support for FunctionChains to Bijectors, if there's interest, e.g. via bi-directioinal conversion in a package extension?

Shouldn't FunctionChains just "work" with Bijectors? Or am I missing something?

oschulz commented 1 year ago

I think you're right @torfjelde - I hadn't noticed that Bijectors.Composed <: Bijector has gone. So Bijectors wouldn't require FunctionChain to be a subtype of Bijector anymore and no conversion is necessary?

torfjelde commented 1 year ago

Exactly:) Now it should "just work" :+1:

oschulz commented 1 year ago

Thanks @torfjelde !