Added Convergence Exceptions for PPCA (EM & Bayes methods) & ICA (fastica!) #37

alexmorley · 2017-05-30T12:40:54Z

Todo

add ConvergenceExceptions to ICA & PPCA methods
~~correct the calculation of the change in model for fastica!~~ Seperate PR.
~~add option to turn off convergence exceptions~~ (can just put tol=Inf)
add extra field to exception (in statsbase) to see how close to convergence the algorithm was
(pending travis) Add lastchange to exceptions now that statsbase is update as above (commit: JuliaStats/StatsBase.jl@4d03581)

nalimilan · 2017-05-30T12:56:18Z

Thanks! I'm not familiar with that code, but the changes look OK to me. However you likely need to increase the minimum StatsBase version (should check when ConvergenceException was introduced).

nalimilan · 2017-05-30T13:36:05Z

It would indeed make sense to print information about how close to convergence the algorithm was. This can easily be added to StatsBase via a new field to ConvergenceException. For backward compatibility, the constructor should have a default value (e.g. NaN) when it's not provided.

alexmorley · 2017-05-30T14:05:33Z

Sorry I deleted my comment above ^^^

Ah yeah OK good idea. I can do that later on. I think it would be a good idea to also add an option to stop these errors being thrown as I have already run into situations where I don't care that much if its converged or not.

ararslan

This looks good, thanks! (And an excellent first contribution, I might add.)

alexmorley · 2017-05-31T19:07:40Z

@ararslan Do you agree with the change in how the change in unmixing matrix is calculated? How it was before wouldn't converge to a near-zero tol? I checked the reference above and the scikit-learn implementation and they both suggest to use the dot product.

ararslan · 2017-05-31T19:17:33Z

test/ica.jl

 @test isa(M, ICA)
 @test indim(M) == m
 @test outdim(M) == k
 @test mean(M) == mv
 W = M.W
 @test W'C * W ≈ eye(k)

+@test_throws StatsBase.ConvergenceException fit(ICA, X, k; do_whiten=true, tol=1)


With the most recent changes, the call to fit does not fail to converge and so the test fails.

ararslan · 2017-05-31T19:22:49Z

I'm not intimately familiar with that algorithm so I can't speak to the merits of using a dot product, but I think that change would be better left to a separate PR.

… algo was

nalimilan · 2017-09-10T15:08:36Z

src/ica.jl

@@ -60,7 +60,7 @@ function fastica!(W::DenseMatrix{Float64},      # initialized component matrix,
                  X::DenseMatrix{Float64},      # (whitened) observation sample matrix, size(m, n)
                  fun::ICAGDeriv,               # approximate neg-entropy functor
                  maxiter::Int,                 # maximum number of iterations
-                  tol::Real,                    # convergence tolerance
+                  tol::Float64,                    # convergence tolerance


Better not restrict accepted types.

This was a workaround because ConvergenceException expects tol & chg to be the same type but can do a conversion instead.

It does not matter. fastica! works only with Float64 data - see above types of W and X parameters. ica should be refactored to accept data of any floating type.

Yeah, but tol could be of a different type from the data itself, as long as comparison operations are supported.

nalimilan · 2017-09-10T15:08:46Z

src/ica.jl

@@ -88,6 +88,7 @@ function fastica!(W::DenseMatrix{Float64},      # initialized component matrix,
    end

    # main loop
+    chg = 0.0


Unused variable?

Will change it to NaN but it makes chg available outside scope of the loop & also (when NaN covers the case of zero iterations.

Got it, thanks!

nalimilan · 2017-09-11T13:06:28Z

test/ica.jl

@@ -49,7 +50,7 @@ C = cov(X, 2)

 # FastICA

-M = fit(ICA, X, k; do_whiten=false)
+M = fit(ICA, X, k; do_whiten=false, tol=Inf)


Why change this?

Because, the way the cost of the model is currently calculated, it's not guaranteed to converge otherwise. As suggested above I could make another PR to change the calculation to use the dot product (see this commit on my fork: alexmorley@f59b63e) and delete the tol=Inf change in the tests in that PR.

Or I could delete it here and make the change from the above commit in this PR.

Does that make sense?

Yes, please make another PR. Mixing different issues is the recipe not to get a PR merged quickly. :-)

The same applies to the i = 0 change elsewhere (which BTW introduces a trailing whitespace).

OK there's another PR in #45 .

nalimilan · 2017-09-11T13:09:37Z

src/ppca.jl

            break
        end
        L_old = L
        i += 1
    end
+    converged || throw(ConvergenceException(tot, chg, Float64(tol)))


Why do you call Float64 here and not above? BTW, it would be better to do oftype(chg, tol) in case chg ends up being of a different type than Float64.

woops... should be on both and will change to use oftype.

nalimilan · 2017-09-11T13:10:04Z

src/ica.jl

@@ -88,6 +88,7 @@ function fastica!(W::DenseMatrix{Float64},      # initialized component matrix,
    end

    # main loop
+    chg = 0.0


Got it, thanks!

nalimilan · 2017-09-11T15:19:04Z

src/ppca.jl

            break
        end
        L_old = L
        i += 1
    end
+    converged || throw(ConvergenceException(tot, chg, oftype(chg,tol)))


Space after comma for consistency.

nalimilan

Thanks! The CI failure is about Compat on Julia 0.5, which is unrelated AFAICT. Probably time to drop 0.5 support.

wildart · 2017-10-09T18:06:04Z

Looks fine.

Added Convergence Exceptions for PPCA (EM & Bayes methods) & ICA

1d8b448

alexmorley changed the title ~~Added Convergence Exceptions for PPCA (EM & Bayes methods) & ICA~~ Added Convergence Exceptions for PPCA (EM & Bayes methods) & ICA (fastica!) May 30, 2017

Update Required StatsBase Version

0e7c84f

nalimilan approved these changes May 30, 2017

View reviewed changes

ararslan approved these changes May 30, 2017

View reviewed changes

ararslan reviewed May 31, 2017

View reviewed changes

alexmorley force-pushed the convergenceException branch from bd133f4 to 0e7c84f Compare June 1, 2017 09:30

alexmorley mentioned this pull request Jul 20, 2017

Convergence exception JuliaStats/StatsBase.jl#284

Merged

alexmorley mentioned this pull request Sep 8, 2017

Tag new versions JuliaStats/StatsBase.jl#298

Closed

updated convergence exception to give info about how close to tol the…

17d4842

… algo was

nalimilan reviewed Sep 10, 2017

View reviewed changes

update (WIP)

23b862d

nalimilan reviewed Sep 11, 2017

View reviewed changes

chgs suggested by @nalimilan

1fcf6d1

nalimilan reviewed Sep 11, 2017

View reviewed changes

Alexander Morley added 2 commits October 9, 2017 11:25

removed trailing whitespace

ea1ea77

undo i = 0 as start of iteration

854396d

alexmorley mentioned this pull request Oct 9, 2017

Changed way the cost of unmixing matrix calculated it ICA #45

Closed

Add space before comma

69fb832

nalimilan approved these changes Oct 9, 2017

View reviewed changes

ararslan merged commit 6f74410 into JuliaStats:master Oct 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Convergence Exceptions for PPCA (EM & Bayes methods) & ICA (fastica!) #37

Added Convergence Exceptions for PPCA (EM & Bayes methods) & ICA (fastica!) #37

alexmorley commented May 30, 2017 •

edited

nalimilan commented May 30, 2017

nalimilan commented May 30, 2017

alexmorley commented May 30, 2017

ararslan left a comment •

edited

alexmorley commented May 31, 2017

ararslan May 31, 2017

ararslan commented May 31, 2017

nalimilan Sep 10, 2017

alexmorley Sep 11, 2017

wildart Sep 11, 2017

nalimilan Sep 12, 2017

nalimilan Sep 10, 2017

alexmorley Sep 11, 2017 •

edited

nalimilan Sep 11, 2017

nalimilan Sep 11, 2017

alexmorley Sep 11, 2017

nalimilan Sep 11, 2017

alexmorley Oct 9, 2017

nalimilan Sep 11, 2017

alexmorley Sep 11, 2017

nalimilan Sep 11, 2017

nalimilan Sep 11, 2017

nalimilan left a comment

wildart commented Oct 9, 2017

Added Convergence Exceptions for PPCA (EM & Bayes methods) & ICA (fastica!) #37

Added Convergence Exceptions for PPCA (EM & Bayes methods) & ICA (fastica!) #37

Conversation

alexmorley commented May 30, 2017 • edited

nalimilan commented May 30, 2017

nalimilan commented May 30, 2017

alexmorley commented May 30, 2017

ararslan left a comment • edited

Choose a reason for hiding this comment

alexmorley commented May 31, 2017

Choose a reason for hiding this comment

ararslan commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexmorley Sep 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan left a comment

Choose a reason for hiding this comment

wildart commented Oct 9, 2017

alexmorley commented May 30, 2017 •

edited

ararslan left a comment •

edited

alexmorley Sep 11, 2017 •

edited