Use keyword syntax for ess, ess_rhat, rhat, and mcse #72
Conversation
src/ess_rhat.jl
Outdated
| if estimator !== nothing && type !== nothing | ||
| throw(ArgumentError("only one of `estimator` and `type` can be specified")) | ||
| elseif estimator !== nothing | ||
| return _ess(estimator, samples; kwargs...) | ||
| elseif type !== nothing | ||
| return _ess(_val(type), samples; kwargs...) | ||
| else | ||
| return _ess(Val(:basic), samples; kwargs...) | ||
| end |
There was a problem hiding this comment.
Could we simplify the logic by just saying, if estimator !== nothing it is used and otherwise the type determines what ESS estimate is returned?
| if estimator !== nothing && type !== nothing | |
| throw(ArgumentError("only one of `estimator` and `type` can be specified")) | |
| elseif estimator !== nothing | |
| return _ess(estimator, samples; kwargs...) | |
| elseif type !== nothing | |
| return _ess(_val(type), samples; kwargs...) | |
| else | |
| return _ess(Val(:basic), samples; kwargs...) | |
| end | |
| if estimator !== nothing | |
| return _ess(estimator, samples; kwargs...) | |
| else | |
| return _ess(_val(type), samples; kwargs...) | |
| end |
(Again, noting that I'm not a big fan of supporting both Val and Symbol)
There was a problem hiding this comment.
I do think it's important to throw an error if the user tries to specify both type and estimator. A bulk-median-ESS is meaningless.
There was a problem hiding this comment.
Hmm I was thinking that it would be sufficient to document that estimator would have higher precedence than type but I can see your point.
That makes me wonder though: Do we actually need two keyword arguments? It seems one would be sufficient if one must not specify both anyway. We could just interpret values of type Symbol (or Val) as the type and everything else as estimator?
There was a problem hiding this comment.
I liked the explicitness of calling it an estimator, especially for consistency with mcse. If we use type for ess as well, then we should also use type for mcse.
There was a problem hiding this comment.
Maybe that would be a good approach? An alternative could be to call it method.
There was a problem hiding this comment.
I prefer not to use method, as to me method is more about how an estimate is made than about what the estimate is (e.g. method currently used for autocovariance estimate method). I merged estimator and type as a new keyword kind. kind is about as semantically overloaded as type but avoids being potentially misunderstood as the type of the return object (also doesn't get colored strangely in syntax highlighters).
Co-authored-by: David Widmann <[email protected]>
| ::Val{:basic}, chains::AbstractArray{<:Union{Missing,Real},3}; split_chains::Int=2 | ||
| ) | ||
| # compute size of matrices (each chain may be split!) | ||
| niter = size(chains, 1) ÷ split_chains |
There was a problem hiding this comment.
It's a bit annoying that the code has (?) to be duplicated. Hopefully at least the tests make it unlikely that they diverge in a noticeable way.
There was a problem hiding this comment.
Yeah, I think we may need a refactor of the code that computes the various variances when we tackle #23, and hopefully then we can eliminate at least part of the redundancy. But yes, the tests should prevent divergences.
| x = _expectand_proxy(estimator, samples) | ||
| if x === nothing | ||
| throw(ArgumentError("the estimator $f is not yet supported by `ess_rhat`")) | ||
| throw(ArgumentError("the estimator $estimator is not yet supported by `ess`")) |
There was a problem hiding this comment.
Makes me wonder, would it be more natural to add a default fallback for _expectand_proxy that does not return nothing but throws the descriptive error message directly?
There was a problem hiding this comment.
Since the error includes the function name, it would seem odd to me to put that error in _expectand_proxy, and the traceback would then point to this internal function not relevant to the user. Since this function is meant only for internal use, I don't think it's a problem for it to have this fallback.
There was a problem hiding this comment.
The second argument is slightly weak though, I think, since also currently the traceback points to an internal function.
There was a problem hiding this comment.
True, but _ess is more similar to ess than _expectand_proxy is.
This PR implements the new keyword-based API proposed in #22 (comment).