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

Widening the scope of the package and dropping support for batching #214

Closed torfjelde closed 1 year ago

torfjelde commented 2 years ago

This PR is an attempt at a couple of things:

  1. Widening the scope, i.e. not being so restrictive regarding whether or not something is bijective.
  2. Removal of expected dimensionality of inputs from the type of the Bijector.
    • This had the benefit of disambiguating whether or not an input represents a single input or a collection of inputs, but it's super annoying to deal with both for the user and implementer + restricts us to only working with arrays, and transformations are expected to preserve dimensionality.
    • After this PR there is no longer "official" support for batching. Some bijectors work for batches because there's no need for disambiguation, but we don't claim that this works for all.
  3. We now also adopt the ChangesOfVariables.jl interface.

TODOs:

torfjelde commented 1 year ago

This is a fairly big one @devmotion , but I would greatly appreciate it if you had a super-quick look. The main part is just removing the dimensionality completely from the definition of the bijectors, in addition to a couple of small things:

  1. Adding mutating methods, e.g. transform!, logabsdetjac!, and with_logabsdet_jacobian! (should this maybe go to ChangesOfVariables.jl?).
  2. Added docs.
  3. Using elementwise(exp) to indicate that a method should be applied elementwise (equivalent to Base.Fix1(broadcast, exp)).

I'm also going to make a separate PR to remove the stuff related to ADbackend.

This will be a huge breaking release, as I think it's time we just rip the band-aid off.

torfjelde commented 1 year ago

@yebai you might also want to take a look at this

torfjelde commented 1 year ago

I think I've replied/addressed your comments now @devmotion

Some I'm of the opinion that we leave until later PRs, given how long this PR has been in the pipeline + a lot of improvements we want to do (e.g. adding a VecCorr bijector) are dependent on this PR making it's way through.

devmotion commented 1 year ago

Yeah, let's just make another breaking release if it becomes necessary. I think I checked yesterday and Bijectors only has ~9 direct dependents, so it's not too bad if we iterate and release multiple breaking versions if needed (of course, it would be better to have the optimal design right away but that's completely unrealistic :smile:).

torfjelde commented 1 year ago

Bueno! You "happy" with the current version of the PR then?

yebai commented 1 year ago

Thanks, @torfjelde @devmotion -- it looks good to me. I agree that we can keep improving the design in new PRs.