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

torfjelde · 2022-02-11T11:16:06Z

This PR is an attempt at a couple of things:

Widening the scope, i.e. not being so restrictive regarding whether or not something is bijective.
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.
We now also adopt the ChangesOfVariables.jl interface.
- There's one issue with this though: https://github.com/TuringLang/Bijectors.jl/pull/183/files#r804539211. A bit uncertain what to do here.

TODOs:

Should we remove (basically) unused and partially unsupported methods: logpdf_with_jac, logpdf_forward, forward(d::Distribution, ...)?
- E.g. forward no longer works for multiple samples because we no longer have support for batched inputs.
- Just removing these seems like the most straight-forward and useful approach atm, so we might as well.

torfjelde · 2023-01-31T17:05:53Z

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:

Adding mutating methods, e.g. transform!, logabsdetjac!, and with_logabsdet_jacobian! (should this maybe go to ChangesOfVariables.jl?).
Added docs.
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.

handling batches

…tors.jl into tor/write-without-batch

torfjelde · 2023-01-31T19:16:44Z

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

docs/src/transforms.md

devmotion · 2023-01-31T23:14:34Z

src/Bijectors.jl

-        InvertibleBatchNorm
+        Coupling,
+        InvertibleBatchNorm,
+        elementwise


Hmm... I wonder if we could reuse some existing functionality in the ecosystem here. And/or if there is a shorter name. Regarding the first point, e.g., Transducers.Map seems similar?

I'm happy to take suggestions, but I'm also okay with elementwise, so IMO this shouldn't hold this PR back.

Not too big of a fan to depend use Transducers.Map though; seems like unnecessary complexity just to make Base.Fix1(broadcast, f).

No, I don't want to depend on Transducers either. Unfortunately, it seems we can't just define a curried version

Base.map(t::Transform) = ...

or

Base.broadcast(t::Transform) = ...

since we would like to use elementwise also for functions such as exp?

since we would like to use elementwise also for functions such as exp?

Exactly 😕

devmotion · 2023-01-31T23:17:13Z

src/Bijectors.jl

+Base.@deprecate NamedBijector(bs) NamedTransform(bs)

-@noinline function Base.inv(b::AbstractBijector)
-    Base.depwarn("`Base.inv(b::AbstractBijector)` is deprecated, use `inverse(b)` instead.", :inv)
-    inverse(b)
-end
+Base.@deprecate Exp() elementwise(exp) false
+Base.@deprecate Log() elementwise(log) false


Are deprecations needed if it is a breaking release? Or would it be sufficient to add them to some changelog/announcement/NEWS.md?

src/bijectors/corr.jl

devmotion · 2023-01-31T23:49:02Z

src/interface.jl

+
+Transform `x` using `b`, treating `x` as a single input.
+"""
+transform(f::Function, x) = f(x)


Not sure if it matters, but Julia won't specialize on f here.

Ah good catch!

src/interface.jl

devmotion · 2023-01-31T23:51:13Z

src/interface.jl

 """
-    isclosedform(b::Bijector)::bool
-    isclosedform(b⁻¹::Inverse{<:Bijector})::bool
+logabsdetjac(b, x) = last(with_logabsdet_jacobian(b, x))


Is this needed? Seems like something that - if desired - should maybe go to ChangesOfVariables.

I personally find this useful, and the last time we spoke about this (JuliaMath/ChangesOfVariables.jl#3), there didn't seem to be a desire to add it 😕

devmotion · 2023-01-31T23:54:12Z

src/interface.jl

+# Useful for checking if compositions, etc. are invertible or not.
+Base.:+(::NotInvertible, ::Invertible) = NotInvertible()
+Base.:+(::Invertible, ::NotInvertible) = NotInvertible()
+Base.:+(::NotInvertible, ::NotInvertible) = NotInvertible()
+Base.:+(::Invertible, ::Invertible) = Invertible()


I would recommend removing these definitions. Seems like a misuse of +.

devmotion · 2023-01-31T23:58:37Z

src/interface.jl

 """
-with_logabsdet_jacobian(b::Bijector, x) = (b(x), logabsdetjac(b, x))
+inverse(t::Transform) = Inverse(t)


Just remove this definition, and it should be sufficient to operate with InverseFunctions.NoInverse instead of Invertible/NoInvertible?

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

…nstead

torfjelde · 2023-02-01T12:20:23Z

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 · 2023-02-01T12:46:14Z

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 😄).

torfjelde · 2023-02-01T14:42:56Z

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

yebai · 2023-02-02T15:39:13Z

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

This PR makes DPPL compatible with the changes to come in TuringLang/Bijectors.jl#214. Tests are passing locally. Closes #455 Closes #456

…eep existing compat) (#469) * Fixed a typo in tutorial (#451) * CompatHelper: bump compat for Turing to 0.24 for package turing, (keep existing compat) (#450) This pull request changes the compat entry for the `Turing` package from `0.21` to `0.21, 0.24` for package turing. This keeps the compat entries for earlier versions. Note: I have not tested your package with this new compat entry. It is your responsibility to make sure that your package tests pass before you merge this pull request. Co-authored-by: Hong Ge <3279477+yebai@users.noreply.github.com> * Some minor utility improvements (#452) This PR does the following: - Moves the `varname_leaves` from `TestUtils` to main module. - It can be very useful in Turing.jl for constructing `Chains` and the like, so I think it's a good idea to make it part of the main module rather than keeping it "hidden" there. - Makes the default `varinfo` in the constructor of `LogDensityFunction` be `model.context` rather than a new `DynamicPPL.DefaultContext`. - The `context` pass to `evaluate!!` will override the leaf-context in `model.context`, and so the current default constructor always uses `DefaultContext` as the leaf-context, even if the `Model` has been `contextualize`d with some other leaf-context, e.g. `PriorContext`. This PR fixes this issue. * Always run CI (#453) I find the current `bors` workflow a bit tedious. Most of the time, I summon `bors` to see the CI results (see e.g. #438). Given that most `CI` tests are quick (< 10mins), we can always run them by default. The most time-consuming `IntegrationTests` is still run by `bors` to avoid excessive CI runs. * Compat with new Bijectors.jl (#454) This PR makes DPPL compatible with the changes to come in TuringLang/Bijectors.jl#214. Tests are passing locally. Closes #455 Closes #456 * Another Bijectors.jl compat bound bump (#457) * CompatHelper: bump compat for MCMCChains to 6 for package test, (keep existing compat) (#467) This pull request changes the compat entry for the `MCMCChains` package from `4.0.4, 5` to `4.0.4, 5, 6` for package test. This keeps the compat entries for earlier versions. Note: I have not tested your package with this new compat entry. It is your responsibility to make sure that your package tests pass before you merge this pull request. Co-authored-by: Hong Ge <3279477+yebai@users.noreply.github.com> * CompatHelper: bump compat for AbstractPPL to 0.6 for package test, (keep existing compat) --------- Co-authored-by: Hong Ge <3279477+yebai@users.noreply.github.com> Co-authored-by: github-actions[bot] <compathelper_noreply@julialang.org> Co-authored-by: Tor Erlend Fjelde <tor.erlend95@gmail.com>

torfjelde added 30 commits June 5, 2021 06:46

renamed rv to result in forward

1d9a1a3

added abstrac type Transform and removed dimensionality from Bijector

0717e3e

updated Composed to new interface

81a2ed6

updated Exp and Log to new interface

251ab9c

updated Logit to new interface

f1ef968

removed something that shouldnt be there

45ff364

removed false statement in docstring of Transform

eb94e00

fixed a typo in implementation of logabsdetjac_batch

0d8783f

added types for representing batches

8f9988e

make it possible to use broadcasting for working with batches

9fa37d1

updated SimplexBijector to new interface, I think

168dd43

updated PDBijector to new interface

d44cf42

use transform_batch rather than broadcasting

c719b07

Merge branch 'master' into tor/rewrite

0962f06

added default implementations for batches

0a62e96

updated ADBijector to new interface

21a66ab

updated CorrBijector to new interface

0b04d14

updated Coupling to new interface

2cfd24b

updated LeakyReLU to new interface

c272cd4

updated NamedBijector to new interface

5e2a585

updated BatchNormalisation to new interface

8e41a50

updated Permute to new interface

9f45b16

updated PlanarLayer to new interface

f793dad

updated RadialLayer to new interface

19d1ef1

updated RationalQuadraticSpline to new interface

5015551

updated Scale to new interface

3b75526

updated Shift to new interface

195a107

updated Stacked to new interface

f56ed6a

updated TruncatedBijector to new interface

72d68b8

added ConstructionBase as dependency

214aa92

torfjelde added 4 commits October 10, 2022 02:12

removed commented code

fc47633

Merge branch 'master' into tor/write-without-batch

f0bc08b

remove reference to logpdf_forward

161b6f1

remove enforcement of type of input and output being the same in tests

ecb54d3

yebai mentioned this pull request Nov 2, 2022

[Merged by Bors] - Linearization/flattening of SimpleVarInfo TuringLang/DynamicPPL.jl#417

Closed

yebai force-pushed the master branch from 8e31c5b to 0bb86e2 Compare January 6, 2023 22:34

Merge branch 'master' into tor/write-without-batch

7450eeb

torfjelde requested a review from devmotion January 31, 2023 17:02

torfjelde added 2 commits January 31, 2023 17:37

make logpdf_with_trans compatible with logpdf when it comes to

269f11a

handling batches

Merge branch 'tor/write-without-batch' of github.com:TuringLang/Bijec…

0e66720

…tors.jl into tor/write-without-batch

torfjelde mentioned this pull request Jan 31, 2023

[Merged by Bors] - Compat with new Bijectors.jl TuringLang/DynamicPPL.jl#454

Closed

devmotion reviewed Jan 31, 2023

View reviewed changes

torfjelde and others added 5 commits February 1, 2023 10:29

Apply suggestions from code review

09fe97d

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

remove usage of invertible, etc. and use InverseFunctions.NoInverse i…

de849ee

…nstead

specialze transform on Function

2ed35b3

removed unnecessary show and deprecation warnings

6ba5a0b

remove references to Log and Exp

fb979a7

devmotion approved these changes Feb 1, 2023

View reviewed changes

torfjelde merged commit 8b924d0 into master Feb 1, 2023

delete-merged-branch bot deleted the tor/write-without-batch branch February 1, 2023 22:47

This was referenced Feb 2, 2023

Rewrite: removing dimensionality and allow non-bijective transformations #183

Closed

Remove ADBackend and ADBijector #242

Merged

bors bot pushed a commit to TuringLang/DynamicPPL.jl that referenced this pull request Feb 3, 2023

Compat with new Bijectors.jl (#454)

0d82a65

This PR makes DPPL compatible with the changes to come in TuringLang/Bijectors.jl#214. Tests are passing locally. Closes #455 Closes #456

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

torfjelde commented Feb 11, 2022 •

edited

Loading

torfjelde commented Jan 31, 2023

torfjelde commented Jan 31, 2023

devmotion Jan 31, 2023

torfjelde Feb 1, 2023

devmotion Feb 1, 2023

torfjelde Feb 1, 2023

devmotion Jan 31, 2023

torfjelde Feb 1, 2023

devmotion Jan 31, 2023

torfjelde Feb 1, 2023

devmotion Jan 31, 2023

torfjelde Feb 1, 2023

devmotion Jan 31, 2023

torfjelde Feb 1, 2023

devmotion Jan 31, 2023

torfjelde Feb 1, 2023

torfjelde commented Feb 1, 2023

devmotion commented Feb 1, 2023

torfjelde commented Feb 1, 2023 •

edited

Loading

yebai commented Feb 2, 2023

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

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

Conversation

torfjelde commented Feb 11, 2022 • edited Loading

torfjelde commented Jan 31, 2023

torfjelde commented Jan 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torfjelde commented Feb 1, 2023

devmotion commented Feb 1, 2023

torfjelde commented Feb 1, 2023 • edited Loading

yebai commented Feb 2, 2023

torfjelde commented Feb 11, 2022 •

edited

Loading

torfjelde commented Feb 1, 2023 •

edited

Loading