Using new Selector type instead of gid

[KDr2]

#567

[xukai92]

Regarding the broken tests, it seems like a deterministic bug simply because we fixed the random seed and use a small number of iterations (500) and small tolerance (0.1). Thus the bug is tied to that particular random seed we set, which makes the estimate slightly larger than the tolerance with 500 iterations. Either simply increasing the number of iterations (e.g. both to 2,000) can fix the tests.

Question is why and where this PR change the random number generator process? With a skim on the changes it doesn't seem direct to me. Or does the use of UUID have any influence on it?

[KDr2]

Question is why and where this PR change the random number generator process? With a skim on the changes it doesn't seem direct to me. Or does the use of UUID have any influence on it?

It must be the UUID, if I change it to a timestamp, all the test cases pass.

[yebai]

Thanks, @KDr2. Excellent work! I've left some questions below. Here are some more for discussions:

• Do we really need a DEFAULT_SELECTOR, since we can always generate a selector for new samplers on-the-fly?
• There's only one place that uses INVALID_SELECTOR, and it's for marking variables for deletion after a variable disappeared from the model during inference. As an alternative, can we treat the deletion as a specific MCMC step, and introduce an implicit sampler that will perform variable elimination. This might be more general since it allows a user to override this implicit sampler with more elaborate operations, e.g. re-use variables after a variable re-appear after several MCMC steps. @xukai92 Again, this can be done in a separate PR.
  • https://github.com/TuringLang/Turing.jl/pull/720/files#diff-8869f2ea8294555b9cd55c4976ae816eR170
• This is less important, and can be done later: we should rename VarInfo.gids to something more readable, perhaps splSelector?

[yebai]

Ready to merge from my side.

[KDr2]

• Do we really need a DEFAULT_SELECTOR, since we can always generate a selector for new samplers on-the-fly?

Currently, the DEFAULT_SELECTOR is used for

1. SampleFromPrior and SampleFromUniform, in this case, we push var into VI with the default selector because these samplers has no selector. This can be easily fixed.
2. In components of Gibbs, their many spl.selector == DEFAULT_SELECTOR and spl.selector != DEFAULT_SELECTOR, I will look into this.

[KDr2]

Ready to merge from my side.

OK, if the PR is merged, I will file new PR for the renaming and DEFAULT_SELECTOR deletion if I can fix them :)

[KDr2]

2. In components of Gibbs, their many spl.selector == DEFAULT_SELECTOR and spl.selector != DEFAULT_SELECTOR, I will look into this.

We use spl.selector ==/!= DEFAULT_SELECTOR to test if a Sampler is a top-level one or a component of another sampler, so if we remove DEFAULT_SELECTOR, we should add a new field to indicate this relation, e.g., a parent field for each sampler. Is it right?

[yebai]

We use spl.selector ==/!= DEFAULT_SELECTOR to test if a Sampler is a top-level one or a component of another sampler, so if we remove DEFAULT_SELECTOR, we should add a new field to indicate this relation, e.g., a parent sampler for each sampler. Is it right?

yes, maybe add a field to Selector instead of Sampler for this purpose? Given that each sampler only has one unique selector field, this might be the most straightforward approach.

[KDr2]

yes, maybe add a field to Selector instead of Sampler for this purpose? Given that each sampler only has one unique selector field, this might be the most straightforward approach.

I think that adding a parent field to the Sampler type may be better:

1. The hierarchy is of the samplers, not the selectors. parent in Sampler type shows this hierarchy more straightforwardly.
2. We can use the singleton SampleFromPrior() as the parent of the top-level samplers.
3. A selector can be reused in different Samplers or be used standalone without any sampler, and selectors have no hierarchies, adding a hierarchy field in it brings much confusion.
4. For now, the argument new_selector in the Sampler constructor is also very confusing. When we use new_selector=true to create a Sampler, we indeed mean to create a Sampler with a parent. So this argument should be changed to parent.

I add some new commits about this, please have a look. @yebai

[yebai]

@xukai92 I'm confused by why we need spl.selector == DEFAULT_SELECTOR here. I understand that this is helpful for HMC samplers to decide whether to perform link!/invlink!, is that the only reason? If so, can we refactor this to remove the requirement of a parent field for samplers? It feels odd to say a sampler is the parent of another sampler since logically they are the same -- i.e. a transition kernel.

[yebai]

Perhaps we can introduce a field to Selector to indicate sampling algorithm types, e.g.

struct Selector
    gid :: UInt64
    alg :: Ref{Symbol} # :default, :invalid, :Gibbs, :HMC, etc.
end
Selector() = Selector(time_ns(), Ref(:default))
Selector(a::Symbol) = Selector(time_ns(), Ref(a))
==(s1::Selector, s2::Selector) = s1.gid == s2.gid

Then we can

• spl.selector == DEFAULT_SELECTOR ==> spl.selector.alg[]==:default
• Remove INVALID_SELECTOR
• and change push!(vi, vn, r, dist, INVALID_SELECTOR) ==>
  push!(vi, vn, r, dist, Selector(:invalid))
  

  Turing.jl/src/inference/pgibbs.jl

  Line 173 in b7ceac2

  push!(vi, vn, r, dist, INVALID_SELECTOR)

This allows the Gibbs constructor to modify Selector field of each component algorithms, and avoid creating an explicit dependence on samplers when initialising Selector.

[KDr2]

Good solution, I will have a try.

[yebai]

Excellent work - thanks @KDr2!

[mohdibntarek]

@KDr2 Sorry, I am late to the party! Thanks for the great PR. I left some comments. If you agree with me, please address them in another PR. Cheers!

[yebai]

@KDr2 Sorry, I am late to the party! Thanks for the great PR. I left some comments. If you agree with me, please address them in another PR. Cheers!

I left some comments above. Other comments look reasonable to me and can be addressed by a new PR.