less code duplication #72
Conversation
Codecov Report
@@ Coverage Diff @@
## master #72 +/- ##
==========================================
- Coverage 95.06% 94.87% -0.19%
==========================================
Files 9 9
Lines 81 78 -3
==========================================
- Hits 77 74 -3
Misses 4 4
Continue to review full report at Codecov.
|
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
That's not always correct though so I'm not sure if it's good to define it automatically. Eg, it's incorrect for the |
|
I agree it's not going to be valid for all likelihoods, but we could define an AbstractScalarLikelihood (or something like that) for which (l::AbstractScalarLikelihood)(fs::AbstractVector{<:Real}) = Product(map(l, fs))would be valid and define it thus? |
Hm, for |
| function (l::CategoricalLikelihood)(fs::AbstractVector) | ||
| return Product(Categorical.(l.invlink.(fs))) | ||
| end |
There was a problem hiding this comment.
is this a bug then @devmotion ?
There was a problem hiding this comment.
looks like this is assuming vectors of vectors (or vectors of tuples)?
There was a problem hiding this comment.
Could the following work then:
(l::AbstractLikelihood)(fs::AbstractVector) = Product(map(l, fs))
(l::CategoricalLikelihood)(f::AbstractVector{<:Real}) = Categorical(l.invlink(f))?
There was a problem hiding this comment.
No, I don't think this is a bug. It is just the usual dispatch we also use in KernelFunctions - multiple samples form a vector, e.g., ColVecs, RowVecs or a vector of vectors. The single sample case is handled by the definition for AbstractVector{<:Real} above.
There was a problem hiding this comment.
I'm happy that this is an improvement over the existing implementations, so I'm happy for it to be merged.
@st-- I'm also happy for this to be rolled in with the 0.4 release associated with #70 as discussed offline, but please only merge this when you're about to merge that PR also, so that we don't leave master hanging with unreleased changes.
Less chance for mistakes / less work when changing anything later on.:)