Added eltype for multivariate distributions #882
Conversation
|
This conflates |
|
@mschauer So I am not sure what you mean by this, but for |
|
@mschauer in that case |
From the constructors it does not seem possible to have different elementary types for the mean and the covariance (also it would not make so much sense). |
|
it would if you are doing Automatic Differentiation on the mean, the covariance would be Float while the mean elements would be Dual |
|
I am not super familiar with the backend of AD but what would be the negative side of having both Dual? |
|
I see the interface exists a while but I think if there is need for a function returning the atomic element type it should not be called |
|
Then rand calls the wrong function ... |
|
|
|
@mschauer are you good with fixing this behaviour here? As mentioned modifying the whole interface can go in another issue |
|
Ok! |
|
ping @theogf can you merge master into this branch and adapt for the comment I let above? |
|
Sorry I am away till next week, I take care of it as soon as I am back. |
|
I'm not familiar with this package, but wanted to note that currently, in However, we are there is a discussion on whether we should have a function |
|
This needs to be rebased. Could someone chime in with a link to a good explanation? |
|
I thought something went wrong because the diff of the branch versus master shows 65 files changed. |
|
ah you're right. @theogf can you from your branch:
This should do it |
Codecov Report
@@ Coverage Diff @@
## master #882 +/- ##
==========================================
+ Coverage 76.45% 76.57% +0.11%
==========================================
Files 108 108
Lines 5157 5161 +4
==========================================
+ Hits 3943 3952 +9
+ Misses 1214 1209 -5
Continue to review full report at Codecov.
|
|
Sorry I completely messed up things with git, the force-push should have got rid of my mistakes |
|
no trouble. There seems to be some misses in codecov, can you check if there are things that should be covered? |
|
ping @theogf |
|
So I wrote some tests, but I quickly started to realize that there are a lot more issues.
Should I just continue pulling the string and parametrize everything until it works? |
| @@ -32,7 +32,10 @@ end | |||
|
|
|||
| GenericMvTDist(df::Real, μ::Vector{S}, Σ::Cov) where {Cov<:AbstractPDMat, S<:Real} = GenericMvTDist(df, μ, Σ, allzeros(μ)) | |||
|
|
|||
| GenericMvTDist(df::T, Σ::Cov) where {Cov<:AbstractPDMat, T<:Real} = GenericMvTDist(df, zeros(dim(Σ)), Σ, true) | |||
| function GenericMvTDist(df::T, Σ::Cov) where {Cov<:AbstractPDMat, T<:Real} | |||
| R = Base.promote_eltype(T, Σ) | |||
There was a problem hiding this comment.
Are you sure Base.promote_eltype is what's needed here?
There was a problem hiding this comment.
I think so, we want a promote_type between T and eltype of Σ and T<:Real
|
A significant amount of code has been added, should there be some additions in the doc? In the tests? |
|
Most of the code has been about relaxing the constructors |
|
Yes agreed
…On Fri, Jun 21, 2019, 14:37 Théo Galy-Fajou ***@***.***> wrote:
Most of the code has been about relaxing the constructors MvTDist to Real
variables and to parametrize the Gamma samplers (previously fixed to
Float64).
Maybe I should add a line in extends.md to suggest to implement eltype
for new distributions.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#882?email_source=notifications&email_token=AB2FDMVCGCRB22FWX3Z7GDTP3TDQHA5CNFSM4HLQNMV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYILA6Y#issuecomment-504410235>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB2FDMX26SCYRVX6O3Y3X5LP3TDQHANCNFSM4HLQNMVQ>
.
|
|
@theogf I'm ok with merging this soon-ish (this week), if you want to add tests before for lines not covered let me know. You can see lines not covered here: |
|
@theogf sorry I let this slept longer than intended, can you merge master into this branch, I'll merge it then |
|
@matbesancon Done! Sorry I also did not answer you, I was first confused by the report and ended up forgetting about it! |
|
ok, this should be good in ~1hour. I think the coverage decreased because of the new |
|
Thanks @theogf |
Should solve #813