More work on `VarNameVector`

[mhauru]

The diff is currently a bit messy because I merged #575 into this. Will clean it up once #575 has been merged into master.

[torfjelde]

Awesome:) Will have a look at this tonight + try to push #575 through. Feel free to just base on #main. My plan was to restart a new PR after #575 anyways.

[mhauru]

Feel free to take a look, but note that it's currently just a draft, I'll keep working on it.

[torfjelde]

Aha yeah tried having a look now and it's a bit much 😅

Might be better to just start off from #575 and cherry pick commits / copy-paste parts from the other PR.

[mhauru]

I did some merge-work, and both the diff from this to #555 and the diff from #555 to master should now be readable and only contain relevant changes.

[mhauru]

Test now pass locally except for the Turing.jl integration tests that I haven't tried yet, and one sampler test which relies on setval!/getval! accepting a sampler rather than a VarName as an argument. I'll need to figure out what to do about the sampler-based indexing (which is what Metadata.gid is for), and compatibility with the old Gibbs sampler. Also needed is a general clean-up and a pass over the TODO notes I've left for myself.

@torfjelde, if you find time to take a look at this at some point, feel free. Doesn't need a full review, but you might have opinions on the direction I've taken this in. We could then merge this into your branch, I would polish it and we would get the whole thing reviewed properly, both your changes and mine mashed together.

[yebai]

I'll need to figure out what to do about the sampler-based indexing (which is what Metadata.gid is for), and compatibility with the old Gibbs sampler.

I'd say leaving them out if that takes a lot of time since we will depreciate the current Gibbs sampler in favour of Experimental.Gibbs soon-ish.

[mhauru]

What exactly is the meaning of the return value of istrans(vi)? Almost all values in a vi are transformed in some way, even if it's only a trivial reshape, or wrapping a scalar in a singleton vector, so in some sense they are all istrans. Does istrans rather mean that they've been transformed into the full Euclidean space?

If yes, I might look into changing the data stored in a VarNamedVector to be something like a domain where the variable lives. Or if that seems like overkill, maybe rename the is_transformed field into something like is_constrained.

[yebai]

Does istrans rather mean that they've been transformed into the full Euclidean space?

yes

Or if that seems like overkill, maybe rename the is_transformed field into something like is_constrained.

I think constrained is more accurate, but need to be careful whether this interacts with #575

[mhauru]

@torfjelde, is there a reason you want to keep the container types in VarNamedVector somewhat free, allowing any subtypes of AbstractVector? I'm somewhat tempted to make it be

struct VarNamedVector{K<:VarName,V,Trans}
    varname_to_index::OrderedDict{K,Int}
    varnames::Vector{K}
    ranges::Vector{UnitRange{Int}}
    vals::Vector{V}
    transforms::Vector{Trans}
    is_transformed::BitVector
    num_inactive::OrderedDict{Int,Int}
end

[mhauru]

SimpleVarInfo{VarNamedVector} is now as well tested as SimpleVarInfo{Dict}. There are cases that it can't handle though, around some complicated use of lenses that probably won't occur often.

Tests pass except for

• A bunch of errors that will be fixed by the next Bijectors.jl release (https://github.com/TuringLang/Bijectors.jl/pull/326)
• Integration tests with Turing.jl, because of Gibbs sampling.

Now that things mostly work, I'll take another read through the code and this time hopefully actually understand it, and clean things up as I go.

[mhauru]

I think this branch, as a way to compare my changes against @torfjelde's, has outlived its usefulness. I'll merge this into Tor's branch, and continue development in #555, with master as the base branch for diffs.