Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic support for uni gmm #615

Merged
merged 13 commits into from
Mar 17, 2019

Conversation

xukai92
Copy link
Contributor

@xukai92 xukai92 commented May 27, 2017

Add generic support for uni-gmm

@ararslan
Copy link
Member

These are now parametric types, you'll have to modify the constructors to account for the type parameter. The 0.5-compatible syntax is

function (::Type{UnivariateGMM{T}}){T}(...)
   ...
end

(Much nicer syntax for this is available in 0.6 but unfortunately it isn't compatible with 0.5.)

@xukai92
Copy link
Contributor Author

xukai92 commented May 29, 2017

Thank you for pointing this. I will make the change soon. I have a problem which I don't know if I can ask here - actually I failed to run the tests for Distributions.jl on my local, with error shown below

Pkg.test("Distributions")
INFO: Computing test dependencies for Distributions...
ERROR: unsatisfiable package requirements detected: no feasible version could be found for package: ColorVectorSpace

Do you know why this happens?

@ararslan
Copy link
Member

I've seen that error once before with a different package. If Pkg.update() shows unexpected warnings, you can always cd ~/.julia/v0.x/METADATA && git fetch origin && git reset --hard origin/metadata-v2 (where 0.x is your current Julia version, e.g. 0.6). If none of that works, I've heard that a solution that works is to delete the contents of ~/.julia/v0.x...

@xukai92
Copy link
Contributor Author

xukai92 commented Jul 27, 2017

Hi Alex, I fix the constructor now.

I stoped working on this PR because I also meet some dependent problems - a group of functions used by uni gmm seems didn't support Dual before, and I didn't manage to fix all of them. But today after I fix the constructor and re-run the them, and the tests seems pass on my local now.

prior::Categorical

function UnivariateGMM(ms::Vector{Float64}, ss::Vector{Float64}, pri::Categorical)
(::Type{UnivariateGMM{T}}){T}(ms::Vector{T}, ss::Vector{T}, pri::Categorical) = begin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change from the long-form function definition? That's generally preferable to begin blocks for style.

end
end

UnivariateGMM{T<:Real}(ms::Vector{T}, ss::Vector{T}, pri::Categorical) = UnivariateGMM{T}(K, ms, ss, pri)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

K isn't defined here

@xukai92
Copy link
Contributor Author

xukai92 commented Jul 28, 2017

Thx Alex. I found that I messed up 3 Distributions.jl forks on my local so I actually run tests on another commit. I will give another try to fix it.

@xukai92
Copy link
Contributor Author

xukai92 commented Jul 28, 2017

Ok, I think I meet the problem I see before: a lot of functions in mixturemodel.jl are not generic.

  • cdf related functions sees not generic support
    • I would like to skip these first and do pdf related frist
  • I separated the test for generic unigmm and commented our cdf related functions now
    • I tried to make some pdf related functions in mixturemodel.jl and now I face some numerical overflow?
julia test/mixture.jl
    testing UnivariateMixture
Test Failed
  Expression: logpdf(g,X[i])  mix_lp0[i]
   Evaluated: -Inf isapprox -2.0463780454938103
ERROR: LoadError: There was an error during testing
 in record(::Base.Test.FallbackTestSet, ::Base.Test.Fail) at test.jl:397
 in do_test(::Base.Test.Returned, ::Expr) at test.jl:281
 in test_mixture(::Distributions.MixtureModel{Distributions.Univariate,Distributions.Continuous,Distributions.Normal}, ::Int64, ::Int64) at mixture.jl:56
 in include_from_node1(::String) at loading.jl:488
 in include_from_node1(::String) at sys.dylib:?
 in process_options(::Base.JLOptions) at client.jl:265
 in _start() at client.jl:321
 in _start() at sys.dylib:?
while loading /Users/kai/projects/Turing/exps/Distributions.jl/test/mixture.jl, in expression starting on line 212

Can you give me some help and some general suggestions how to make things in mixturemodel.jl generics

@matbesancon
Copy link
Member

Hi @xukai92 what's the status on this? Maybe you can keep going on this PR on Julia 1.0 and the latest Distributions master branch

@matbesancon
Copy link
Member

ping @xukai92 to move on this :)

@xukai92
Copy link
Contributor Author

xukai92 commented Feb 28, 2019

@matbesancon Thanks for pinging me! I'm on it now :)

@xukai92
Copy link
Contributor Author

xukai92 commented Feb 28, 2019

I disabled the tests for generic support of GMM for rand(g, n) as this is not supported for Normal, i.e. below fails:

julia> d = Normal(Dual(0), Dual(1))
Normal{Dual{Nothing,Int64,0}}=Dual{Nothing}(0), σ=Dual{Nothing}(1))

julia> rand(d, 10)
ERROR: MethodError: no method matching Float64(::Dual{Nothing,Float64,0})
Closest candidates are:
  Float64(::Real, ::RoundingMode) where T<:AbstractFloat at rounding.jl:194
  Float64(::T<:Number) where T<:Number at boot.jl:741
  Float64(::Int8) at float.jl:60
  ...
Stacktrace:
 [1] convert(::Type{Float64}, ::Dual{Nothing,Float64,0}) at ./number.jl:7
 [2] setindex!(::Array{Float64,1}, ::Dual{Nothing,Float64,0}, ::Int64) at ./array.jl:767
 [3] rand! at /Users/kai/projects/Distributions.jl/src/univariates.jl:167 [inlined]
 [4] rand at /Users/kai/projects/Distributions.jl/src/univariates.jl:160 [inlined]
 [5] rand(::Normal{Dual{Nothing,Int64,0}}, ::Int64) at /Users/kai/projects/Distributions.jl/src/genericrand.jl:24
 [6] top-level scope at none:0

@xukai92
Copy link
Contributor Author

xukai92 commented Feb 28, 2019

@matbesancon Not sure why some tests for Hypergeometric fail.

@matbesancon
Copy link
Member

yes sadly we need #836 merged for this first

@matbesancon
Copy link
Member

not sure if we should do it here or in a following PR, but it would be good to replace Vector by a new implementation

@xukai92
Copy link
Contributor Author

xukai92 commented Mar 6, 2019

Do yo mean making everything typed by AT, T where AT<:AbstractVector{T}, T<:Real?

@matbesancon
Copy link
Member

yes exactly

@matbesancon
Copy link
Member

Also what was done in #837 for multivariate gaussians

@matbesancon
Copy link
Member

ping @xukai92 all good on this?

@@ -135,13 +135,13 @@ function MixtureModel(components::Vector{C}, prior::Categorical) where C<:Distri
MixtureModel{VF,VS,C}(components, prior)
end

MixtureModel(components::Vector{C}, p::Vector{Float64}) where {C<:Distribution} =
MixtureModel(components::Vector{C}, p::Vector{T}) where {C<:Distribution,T<:Real} =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we allow for any <:AbstractVector{<:Real}?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only place we need to change? Or we actually need make AT<: AbstractVector part of the parameter of type MixtureModel?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes exactly

src/mixtures/unigmm.jl Outdated Show resolved Hide resolved
src/mixtures/unigmm.jl Outdated Show resolved Hide resolved
src/mixtures/unigmm.jl Outdated Show resolved Hide resolved
@xukai92
Copy link
Contributor Author

xukai92 commented Mar 14, 2019

@matbesancon suggestions fixed! Please take another review.

@matbesancon matbesancon requested a review from ararslan March 15, 2019 09:00
@matbesancon
Copy link
Member

Approved, merged in 12 hours if no objection

@matbesancon matbesancon requested a review from mschauer March 15, 2019 09:05
@xukai92
Copy link
Contributor Author

xukai92 commented Mar 15, 2019

Thanks for the help @matbesancon !

Copy link
Member

@mschauer mschauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objection.

@matbesancon matbesancon merged commit 73e35a2 into JuliaStats:master Mar 17, 2019
@matbesancon
Copy link
Member

woops, 12 hours became 2 days, merged. Thanks for the contribution @xukai92

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants