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

:multmse fails; returns zeros all the time #16

Closed
montyvesselinov opened this issue Oct 9, 2016 · 14 comments
Closed

:multmse fails; returns zeros all the time #16

montyvesselinov opened this issue Oct 9, 2016 · 14 comments

Comments

@montyvesselinov
Copy link

montyvesselinov commented Oct 9, 2016

julia> for T in (Float64, Float32)
           Wg = max(rand(T, p, k) .- 0.3, 0)
           Hg = max(rand(T, k, n) .- 0.3, 0)
           X = Wg * Hg

           for alg in (:multmse, :multdiv, :projals, :alspgrad)
               for init in (:random, :nndsvd, :nndsvda, :nndsvdar)
                   ret = NMF.nnmf(X, k, alg=alg, init=init); @show ret;
               end
           end
       end
ret = NMF.Result{Float64}([0.0 0.0 0.0
 0.0 0.0 0.0
 0.0 0.0 0.0
 0.0 0.0 0.0
 0.0 0.0 0.0],[0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0
 0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0
 0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0],2,true,2.716738442005015)
ret = NMF.Result{Float64}([0.0 0.0 0.0
 0.0 0.0 0.0
 0.0 0.0 0.0
 0.0 0.0 0.0
 0.0 0.0 0.0],[0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0
 0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0
 0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0],2,true,2.716738442005015)
@ararslan
Copy link
Member

ararslan commented Oct 9, 2016

Perhaps related to #15...

@ararslan
Copy link
Member

ararslan commented Oct 9, 2016

Interestingly it looks like you're executing the code that we have in the test directory, so in theory that should be an active test, but it's not actually using Base.@test, so nothing is being tested (other than that it doesn't burst into flames, I suppose).

@montyvesselinov
Copy link
Author

I agree; the testing of this module (NMF) is not acceptable.

-monty

On Sun, Oct 9, 2016 at 11:59 AM, Alex Arslan notifications@github.com
wrote:

Interestingly it looks like you're executing the code that we have in the
test directory, so in theory that should be an active test, but it's not
actually using Base.@test, so nothing is being tested (other than that it
doesn't burst into flames, I suppose).


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#16 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFXxeLe3y2chCwoqUqyj_8c9jYMVE4Ueks5qySt9gaJpZM4KSCeA
.

@mirestrepo
Copy link

Is there any response to the behavior here or in issue #15? Is this package actively maintained?

@ararslan
Copy link
Member

ararslan commented May 1, 2017

I'm not sure yet what the cause is and I haven't had much time to look into it. I'll investigate it more soon and see if I can figure out what's going on.

@mirestrepo
Copy link

... it only seems to happen when using the nnmf "interface"

For instance

using NMF

function test_interface()
    srand(5678)

    p = 5
    n = 8
    k = 3

    T = Float64
    Wg = max(rand(T, p, k) .- 0.3, 0)
    Hg = max(rand(T, k, n) .- 0.3, 0)
    X = Wg * Hg

    res = NMF.nnmf(X, k; init=:nndsvdar, alg=:multdiv)

    println(res)
end

function test_explicit()
    srand(5678)

    p = 5
    n = 8
    k = 3

    T = Float64
    Wg = max(rand(T, p, k) .- 0.3, 0)
    Hg = max(rand(T, k, n) .- 0.3, 0)
    X = Wg * Hg

    W, H = NMF.nndsvd(X, k; variant= :ar)
     # optimize
    alginst = NMF.MultUpdate{Float64}(obj=:div, maxiter=100, verbose=false)
    res = NMF.solve!(alginst, X, W, H)

    println(res)
end

println("------Test Interface----------")
test_interface()
println("------Test Explicit----------")
test_explicit()

Gives me

------Test Interface----------
NMF.Result{Float64}([NaN NaN NaN; NaN NaN NaN; NaN NaN NaN; NaN NaN NaN; NaN NaN NaN],[0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0; 0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0; 0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0],1,true,NaN)
------Test Explicit----------
NMF.Result{Float64}([0.552115 1.97784e-32 1.9149; 0.127375 0.208774 1.70371; 0.0740455 1.36287 0.174001; 0.0740158 1.37869 1.00644e-5; 0.572561 8.63124e-72 1.51597e-63],[2.70055e-65 0.707858 0.508 6.7034e-21 2.8746e-21 4.26979e-65 0.652906 0.641621; 0.156444 0.112416 0.000632513 0.156245 0.0670022 0.247222 9.12524e-20 0.160251; 1.40802e-37 0.132208 3.00506e-5 0.208056 0.0892199 1.41131e-37 7.683e-9 0.0486252],100,false,0.005155024524540801)

@mirestrepo
Copy link

I found the reason for the discrepancy. In the interface, for all algorithms other than :projals, the H matrix is initialized to zeros... in my test_explicit() that was not the case. This makes me believe there must be a division by zero happening somewhere...

@mirestrepo
Copy link

mirestrepo commented May 1, 2017

I think I found the bug!. The README says

For some algorithms (e.g. ALS), only W needs to be initialized. For such cases, one may set the keyword argument zerohto be true, then in the output H will be simply a zero matrix of size (k, n).

But in the code interf.jl - line 14

initH = alg == :projals
zeroh=!initH

That does exactly the opposite - it's setting H to zero for all other algorithms, when it should only be doing it for ALS!

@lindahua I think you wrote the higher level interface. Could this all make sense?

@montyvesselinov
Copy link
Author

this is still not fixed. Maybe we should do a pull request. Or give up.

@ararslan
Copy link
Member

Sorry, this package is not really under active maintenance these days. I looked into this a while back but wasn't able to figure it out, then got busy with other things. If you know of a fix, a PR would be more than welcome!

@mirestrepo
Copy link

mirestrepo commented Jul 26, 2017

I can try to work a pull request in the next few days - I think the fix is simple, the problem is the package doesn't have testing that I could immediately have used to make sure changes were okay. I'll try to write some basic tests ....

@mirestrepo
Copy link

mirestrepo commented Jul 26, 2017

My solution in the mean time was to skip the global interface, as the algorithm specific interface seems to give sensible results for a simple exploration I did http://nbviewer.jupyter.org/github/bcbi/julia_tutorials/blob/biol1555_spring17/nnmf/nnmf_classic3_clustering.ipynb

@montyvesselinov
Copy link
Author

Sounds good; BTW, the performance of the algorithm is very good in general.

korenmiklos added a commit to korenmiklos/NMF.jl that referenced this issue Sep 19, 2018
Projected ALS does not need H initialized, all others do
@ghost
Copy link

ghost commented Oct 23, 2020

@ararslan Pull request #25 is critical. If H is not initialized properly, the performance of nndsvd might be degraded. I think the pull request should be merged. Please consider it.

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

No branches or pull requests

3 participants