Rename `logpdf_with_trans` to `logpdf_with_jacobian`? about bijectors.jl HOT 6 OPEN

I agree with changing it, as I initially got quite confused by the name myself. What confused me was the fact that the logpdf_with_trans is computing the logpdf of the transformed variable using the non-transformed one. From both the current name and the one suggested by @willtebbutt I immediately think it's actually doing logpdf(d, invlink(d, y)) + logabsdetjac(invlink, y) rather than logpdf(d, x) - logabsdetjac(link, x), where y is the transformed variable and x = rand(d), if that makes sense. It's of course the same result, but what's the expected input was kind of confusing to me.

With that being said, I realize such changing the internals of the methods might be a bit drastic given other packages' dependence on Bijectors.jl. So in that case, I think I prefer logpdf_with_jac or logpdf_with_jacobian, as it doesn't give such a strong indication that the expected input is the transformed variable when it isn't.

Worth noting that I already introduced logpdf_with_jac in the recent interface PR, which returns a tuple (logpdf_with_trans, logjac). But we also recently introduced a forward(d::Distribution) which kind of replaces logpdf_with_jac and is more useful, so we could easily drop that method and use the name for this purpose instead.

from bijectors.jl.

Perhaps logpdf_of_trans? i.e. the logpdf of the transformed random variable?

from bijectors.jl.

That sounds good too.

from bijectors.jl.

@xukai92 @torfjelde Does Will's proposal make sense to you?

from bijectors.jl.

I prefer logpdf_with_jac.

Also compared with forward, logpdf_with_jac is more verbose - when people reads the code they at least know it's doing something about computing the log density with some Jacobian computation invovled.

from bijectors.jl.

logpdf_with_jac looks good to me!

from bijectors.jl.