Fix remaining method ambiguities

[penelopeysm]

This PR:

1. Enables Aqua method ambiguity tests (as per Enabling aqua ambiguity testing for Turing #2290)
2. Creates an AbstractTransition abstract type which all the Transitions in Turing subtype.
3. Modifies the type signature of bundle_samples (in src/mcmc/Inference.jl) to take a Vector{<:Union{AbstractTransition,AbstractVarInfo}} as the first argument. The AbstractVarInfo case occurs when sampling with Prior(), so the type signature of this argument somewhat mirrors that of the sampler argument in the same function (which is Union{Sampler{<:InferenceAlgorithm},SampleFromPrior}).
4. Modifies the type signature of Base.get (in src/optimisation/Optimisation.jl) to take an AbstractVector{Symbol} as the second argument.

(4) is technically a breaking change because it is a more restrictive type signature, hence I've also bumped the version number to 0.34.0.

Closes #2261; extends #2290.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.79%. Comparing base (5b5da11) to head (75241bf).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2304      +/-   ##
==========================================
- Coverage   86.85%   86.79%   -0.07%     
==========================================
  Files          24       24              
  Lines        1598     1598              
==========================================
- Hits         1388     1387       -1     
- Misses        210      211       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[coveralls]

Pull Request Test Coverage Report for Build 10633882336

Details

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage decreased (-0.06%) to 87.014%

---

Files with Coverage Reduction	New Missed Lines	%
ext/TuringOptimExt.jl	1	98.04%

Totals
Change from base Build 10629147969:	-0.06%
Covered Lines:	1387
Relevant Lines:	1594

---

💛 - Coveralls

[mhauru]

The code looks good to me. I started to wonder about where AbstractTransition should live, and whether things might break for packages that build their own samplers with their own transition types.

My first observation is that there is a type called AbstractTransition in AbstractMH: https://github.com/TuringLang/AdvancedMH.jl/blob/fb7e87280fd5f1ad44aaca4889d8bcd48cb739e7/src/AdvancedMH.jl#L33 Is that what we want here? Or do we want to define something even more general in AbstractMCMC, that the AbstractMH one would be a subtype of? I don't really understand what transitions are, so someone else probably needs to answer this.

Also, here's a list of types I found in TuringLang that seem to implement something like a transition. Does it bring sense to bring all under the same abstract type? Would this PR cause breakage for them if they try to call bundle_samples?
https://github.com/TuringLang/SliceSampling.jl/blob/25c9d1845f8125a2660fd52b76f1f660426817b6/src/SliceSampling.jl#L25
https://github.com/TuringLang/AdvancedHMC.jl/blob/2b3814ccbb36806eb0f21c2c937146606a873884/src/trajectory.jl#L12
https://github.com/TuringLang/MCMCTempering.jl/blob/33414c09fd848667180ac77a8c3f22abaea73240/src/samplers/multi.jl#L64
https://github.com/TuringLang/MCMCTempering.jl/blob/33414c09fd848667180ac77a8c3f22abaea73240/src/tempered_sampler.jl#L19
https://github.com/TuringLang/MCMCTempering.jl/blob/33414c09fd848667180ac77a8c3f22abaea73240/src/samplers/composition.jl#L69
https://github.com/TuringLang/MCMCTempering.jl/blob/33414c09fd848667180ac77a8c3f22abaea73240/src/swapsampler.jl#L242
https://github.com/TuringLang/AdvancedMH.jl/blob/fb7e87280fd5f1ad44aaca4889d8bcd48cb739e7/src/MALA.jl#L14
https://github.com/TuringLang/MCMCTempering.jl/blob/33414c09fd848667180ac77a8c3f22abaea73240/src/abstractmcmc.jl#L60

[mhauru]

@sunxd3, @yebai, any thoughts on the above?

[yebai]

It might cause issues but @devmotion, @torfjelde probably have thought more about this.

[devmotion]

My gut feeling is that right now an AbstractTransition has only limited use since all these transitions have different fields and even different field names for the samples (e.g., different in Turing compared with AdvancedMH), and hence if you dispatch on it as in the bundle_samples functions you must rely on some (Turing-internal) functions for extracting log evidence etc.

I think it could be useful to add an interface for accessing the most important properties of a transition to AbstractMCMC + a default Transition implementation (it seems, in Turing and AdvancedMH it's mainly samples, log density + some statistics) that downstream packages could use instead of defining their own types. Possibly, in most cases such a default Transition object could be sufficient? If not, I think AbstractMCMC would also be a natural location for an abstract supertype such as AbstractTransition.

[sunxd3]

To echo David's point, I think a default Transition type defined in AbstractMCMC would be good. But for now, for the sake of resolving ambiguities, I am okay with this PR.

[penelopeysm]

I agree that the fields of the different Transitions don't define a common interface (yet) — the only usefulness of defining an abstract supertype would be to fix the method ambiguity and maybe a small improvement in code readability.

I'm in favour of having an overarching AbstractTransition type in AbstractMCMC! I think, though, that will run a bigger risk of breaking some of the code examples that @mhauru brought up, and so it'd need to be done very carefully as a larger piece of work.

@mhauru: Regarding your point about other libraries: Right now some libraries in the Turing ecosystem have their own kind of Transition, e.g. AdvancedHMC and AdvancedMH, and some of them also define their own version of bundle_samples that dispatch on their own Transition type. I think the way this works out is that they will either call their own version of bundle_samples or (if it's not defined) the default AbstractMCMC implementation, but not an implementation within Turing core.

I'm less certain about this, but I think it's also not possible to call Turing's bundle_samples with a Transition from a different library as Turing's bundle_samples is restricted to a certain set of samplers that are defined within Turing itself (and the samplers will define the Transition type).

(Please correct me if I'm wrong, I'm finding a lot of this quite difficult to reason about due to the complexity of the ecosystem + the fact that Julia lets you extend methods in other packages.)

[yebai]

I'm in favour of having an overarching AbstractTransition type in AbstractMCMC! I think, though, that will run a bigger risk of breaking some of the code examples that @mhauru brought up, and so it'd need to be done very carefully as a larger piece of work.

@penelopeysm, can you open an issue on AbstractMCMC to track this discussion about creating AbstractTransition? We might keep discussing it for a while before deciding whether to actually implement it.

(Please correct me if I'm wrong, I'm finding a lot of this quite difficult to reason about due to the complexity of the ecosystem + the fact that Julia lets you extend methods in other packages.)

Your understanding is correct; hopefully, the complexity will decrease when you get more familiar with Turing and Julia. It will be even better if we simplify the design, e.g. DynamicPPL/AbstractPPL/JuliaBUGS.

[sunxd3]

looks good to me