match inner and outer keys #46
Conversation
Codecov Report
@@ Coverage Diff @@
## main #46 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 89 96 +7
=========================================
+ Hits 89 96 +7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -54,6 +54,12 @@ for T in (:Distribution, :Sampleable) | |||
| "lengths of key vectors $key_lengths must match " * | |||
| "size of distribution $(_size(d))" | |||
| )) | |||
| if d isa Distribution && mean(d) isa KeyedArray && !(axiskeys(mean(d)) == keys) | |||
There was a problem hiding this comment.
won't we run into the same issue with cov? or any other function that might be return a KeyedArray?
Would it be better to rekey the underlying parameters and just mask the exception altogether?
I feel like wrapping a MvN around a KeyedArray, then wrapping that in a KeyedDistribution is akin to rekeying anyway?
There was a problem hiding this comment.
It looks like we won't need to look at cov actually
julia> mvn = MvNormal(ka, sigma);
julia> mean(mvn)
1-dimensional KeyedArray(NamedDimsArray(...)) with keys:
↓ t ∈ 3-element Vector{String}
And data, 3-element Vector{Float64}:
("a") 0.3981265148452625
("b") 0.23250652731094346
("c") 0.09181862970601418
julia> cov(mvn)
3×3 Matrix{Float64}:
0.0702741 0.0 0.0
0.0 0.689994 0.0
0.0 0.0 0.800233
# which is because of
julia> cholesky(sigma)
Cholesky{Float64, Matrix{Float64}}
U factor:
3×3 UpperTriangular{Float64, Matrix{Float64}}:
0.265093 0.0 0.0
⋅ 0.830659 0.0
⋅ ⋅ 0.894558and yes this
Would it be better to rekey the underlying parameters and just mask the exception altogether?
is a good idea
There was a problem hiding this comment.
Would it be better to rekey the underlying parameters and just mask the exception altogether?
I think this is actually quite hard. I think we would have to recreate the underlying distribution with the keyless_unnamed arrays. The issue is that constructors for distributions differ. We could possibly do something nasty like keyless_unname.(fieldnames...) and call the default constructor but it sounds like asking for trouble.
Does that leave us with throwing an exception? Or should we simply continue ostriching the issue of mismatching inner and outer keys?
There was a problem hiding this comment.
I think it would be good to prevent confusion where possible, so support having a
- add the
meansanity check to give a warning/exception generally - rekey for common distributions such as
MvNormal(with warning)
Support can then be extended to other distributions as need arises, and we can document the care that should be taken when using a general distribution with keyed fields
Closes #41