TuringLang / Bijectors.jl

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

Rename `rv` from `forward(b::Bijector, x)` result #41

Open torfjelde opened 5 years ago

torfjelde commented 5 years ago

At the moment, if you call forward(b, x) you get back a named tuple (rv = b(x), logabsdetjac = logabsdetjac(b, x)). This was introduced in the first PR for the new interface.

When I started out working on this, using rv made sense as the transform was always related to some Distribution. Now a Bijector can be used as a standalone transform to do more than just transform random variables, e.g. constrained-to-unconstrained transformations in optimization. Therefore I suggest we rename rv.

A couple of suggestions (in no particular order):

  1. y since throughout the entire package it's a recurring theme where x is "un-transformed" and y denotes transformed. Also, in forward, x is the input. This also corresponds with the (x = x, y = b(x), ...) returned by calling forward(d::TransformedDistribution). Though could also be up for discussion in this issue.
  2. val or value
  3. res or result

Any suggestions?

xukai92 commented 5 years ago

I like 1 and 3. The reason I don't like 2 is because val or value doesn't indicate the value is transfomed or not.

willtebbutt commented 5 years ago

I vote 1

torfjelde commented 5 years ago

Personally I also like 1. One possible issue is that one can also call forward(ib::Inversed{<:Bijector}, y). Should this also return y = ...? We should use the same key in forward for both inversed and not, imo.

willtebbutt commented 5 years ago

Another alternative is output. I guess y isn't such a great name if you consider inverse transformations.

torfjelde commented 5 years ago

Another alternative is output. I guess y isn't such a great name if you consider inverse transformations.

True. I think I prefer 3 over output but not by much

xukai92 commented 5 years ago

Similarly, ans is a good choice. It's also Julia's default name holder for results.

➜  ~ julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.1.0 (2019-01-21)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |
julia> 1 + 1
2

julia> ans
2
trappmartin commented 5 years ago

I prefer 3.

torfjelde commented 4 years ago

A final call for votes: @yebai @cpfiffer @mohamed82008 @sharanry

yebai commented 4 years ago

I prefer 3 as well.

torfjelde commented 4 years ago

Then 3 it is!

People comfortable with renaming rv to result then? I think it's okay to be slightly verbose since you can also do y, logjac = forward(b, x) for that sweet conciseness.

Thumbs up if you agree, thumbs down if you prefer res.