Implementing a custom bijector is a hassle: solve by adding macro? about bijectors.jl HOT 3 OPEN

IMO a macro seems to complicated and leads to un-julian syntax. I also don't think it is necessarily easier for users to figure out how to write the macro than just implementing the three functions currently. I'm not sure if forward is part of the API or just an implementation detail, and how often it happens that . In any case, I think maybe the following could work:

• Define all methods that apply bijectors to multiple inputs (i.e., vectors, matrices, etc.) generically for all bijectors since we do not support Julia < 1.3 anymore.
• Use something like @thunk in ChainRulesCore (see https://www.juliadiff.org/ChainRulesCore.jl/dev/writing_good_rules.html#Use-Thunks-appropriately and https://www.juliadiff.org/ChainRulesCore.jl/dev/api.html#ChainRulesCore.Thunk) to implement the following default:

(f::Bijector)(x) = unthunk(forward(f, x).rv)
logabsdetjac(f::Bijector, x) = unthunk(forward(f, x).logabsdetjac)

(BTW I'm not sure about these names, maybe just make it a tuple or use something else than rv?)

• Let users (mainly) implement

function Bijectors.forward(f::MyCoolBijector, x)
	....
	return (rv = ..., logabsdetjac = ...)
end

possibly using @thunk

• Add implementations of a reduced logabsdetjac version for and loglikelihood_with_trans (see #120) that avoid the intermediate allocations of arrays in logabsdetjac

from bijectors.jl.

IMO a macro seems to complicated and leads to un-julian syntax.

But the "un-Julian syntax" is mostly due to the fact that we're dropping the return statement, right? If so, we could just make the user add it manually, i.e. return (rv = rv, logabsdetjac = logabsdetjac), or make this optional. Other than that, there's not much un-julian about the macro, IMO.

I also don't think it is necessarily easier for users to figure out how to write the macro than just implementing the three functions currently.

That's true but the goal here isn't to make it easier to understand, but easier to go from "I want this bijector" to "I have this bijector". Using a macro we could make it so that there is a minimal amount of work on the user, in addition to getting the most efficient implementation for all the necessary functions. E.g. RadialLayer and PlanarLayer would require waaay less code in addition to being more efficient than the current implementation (the fact that these unnecessarily compute logabsdetjac in the (b::Bijector)(x) method, kind of proves the point that people aren't bothered to implement all the different methods, haha).

I'm not sure if forward is part of the API or just an implementation detail, and how often it happens that .

It's part of the API 👍 And there are cases where it's definitively worth it, e.g. RadialLayer for high-dimensional input. It's not particularly useful for stuff like Exp, but once you start working with 500-dimensional normalizing flows this becomes very important.

Define all methods that apply bijectors to multiple inputs (i.e., vectors, matrices, etc.) generically for all bijectors since we do not support Julia < 1.3 anymore.

I'm potentially for this. But it's worth noting that Bijectors.jl still works for Julia <1.3, it's just that we don't test properly + certain AD-backends doesn't work. This introduction would completely break Bijectors.jl for Julia <1.3.

Use something like @thunk in ChainRulesCore

You're thinking along the lines of

function _forward(b, x)
    rv = @thunk ...
    logabsdetjac = @thunk ...
    return (rv = rv, logabsdetjac = logabsdetjac)
end

forward(b, x) = unthunk(_forward(b, x))
(b::Bijector)(x) = unthunk(_forward(b, x).rv)
logabsdetjac(b, x) = unthunk(_forward(b, x).logabsdetjac)

right?

I'd argue that this is both a) more complicated to understand for the user, b) way worse performance as closures have comparatively significant overhead.

Add implementations of a reduced logabsdetjac version for and loglikelihood_with_trans (see #120) that avoid the intermediate allocations of arrays in logabsdetjac

I'm in favour of the suggestion, but it seems like a slightly different issue, no?

from bijectors.jl.

(BTW I'm not sure about these names, maybe just make it a tuple or use something else than rv?)

Agree, but also separate issue. We discussed renaming rv to res (#41) but the issue sort of lost it's momentum and we ended up getting stuck with it.. And I'm not certain about just making it a tuple, since NamedTuple means you can access it both using indexing and .varname.

from bijectors.jl.