AbstractChains interface #180
Conversation
|
@yebai Tagging you for overall comments on the approach. |
|
AbstractMCMC.jl documentation for PR #180 is available at: |
| using Test | ||
|
|
||
| """ | ||
| AbstractMCMC.Chains.get_data(chn, key) |
There was a problem hiding this comment.
Since this function always returns AbstractMatrix, maybe rename it to to_array for clarity:
| AbstractMCMC.Chains.get_data(chn, key) | |
| AbstractMCMC.Chains.to_array(chn, key) |
There was a problem hiding this comment.
It's more like a dictionary get because it requires a key. IMO, to_array sounds like you're converting the entire chain to an array.
| """ | ||
| AbstractMCMC.Chains.iter_indices(chn) | ||
|
|
||
| Obtain the indices of each iteration for the `AbstractChains` object `chn`. |
There was a problem hiding this comment.
This is not very clear to me. Maybe add a working example to this and related APIs below for readability?
There was a problem hiding this comment.
I don't really see how to add an example without a concrete implementation. I'll flesh out the docstring though.
| end | ||
| end | ||
|
|
||
| # Plotting functions; to be extended by individual chain libraries |
There was a problem hiding this comment.
Not sure we want to define the plotting APIs in AbstractMCMC. Let's focus on Chain type and core interface only.
There was a problem hiding this comment.
The problem here is if AbstractMCMC owns the chains type, then any plotting recipe that uses it also has to belong in AbstractMCMC. Otherwise, if you shifted the plotting functionality to a separate library, then defining something like plot(chn::AbstractChains) or density(chn::AbstractChains) would be type piracy, since AbstractChains is owned by AbstractMCMC.jl, and plot or density are owned by Plots.jl. This is the same issue that ChainsMakie has/had. If you wanted to move the plotting functionality to a separate library, then I would strongly suggest that everything should be in that separate library i.e. just make AbstractChains.jl hold all of it.
Of course, there are differing levels of type piracy. Maybe it's not so bad for a package in TuringLang to pirate a type that is owned by a separate package in TuringLang. But I still think it's bad.
2 participants
This PR adds some private sections of the AbstractChains interface.
The current interface consists of three things: the
AbstractChainsabstract type,chainscat, andchainsstack. These are preserved as part of the public interface, which can be directly accessed asAbstractMCMC.(...)and are insrc/chains.jl.The new portion of the interface contains a number of new functions which are explicitly considered experimental and thus subject to change (see the docstrings). These must all be accessed via
AbstractMCMC.Chains.(...)and are insrc/experimental/chains.jl.Since nothing here is breaking (none of the existing tests failed, I added one more test for the AbstractChains interface) it's just a minor version bump.
This PR alone will resolve the immediate name conflicts arising from plotting functions TuringLang/Turing.jl#2681 (we don't need to have concrete implementations of them, we just need to have a shared function exported from one place).