Fix omitted ids, write tests.

[rgiordan]

At some point the usage of omitted ids got broken. There are no unit tests, either.

julia> DeterministicVI.maximize_f(elbo_fft_opt, ea_fft, omitted_ids=[1])

ERROR: BoundsError: attempt to access 28-element Array{Float64,1} at index [29,1]
 in (::Celeste.DeterministicVI.##33#38)(::Array{Float64,1}, ::Array{Float64,1}) at /home/rgiordan/.julia/v0.5/Celeste/src/deterministic_vi/maximize_elbo.jl:67

[jeff-regier]

@jrevels is work on refactoring Transform.jl now. I was thinking we'd get rid of omitted_ids and keep_ids because of the complexity, and because it doesn't seem as necessary any more. If we want to restore that functionality, the way to do it is probably to define another transform, like the box constraints or the simplex constraint that removes certain ids.

Are you using omitted_ids often? Is restoring this functionality a high priority? (It would take resources away from other things.)

[rgiordan]

Doing it through a transform would be fine, but I do think you should implement it. I spent some time digging into why the optimization problem is slow. Part of that was digging into why the ELBO Hessian so tightly couples the galaxy and star parameters. It turns out that the log term, per our intuition, actually doesn't couple the parameters much, but the shared location parameter does. I then watched some animations of the optimization path, and it occurred to me that along the way to getting the position correct, it is constantly updating the galaxy shape parameters, too, since having the implied position wrong will also bias the size of the galaxy outwards in order to capture the uneven distribution of light caused by the location being incorrect. That suggests optimizing in two steps: first, keeping the galaxy parameters fixed as if they were a small star, setting a = [1, 0], and optimizing only the star parameters. Even if it's a galaxy, since galaxies are symmetric, this should be a reasonable guess of the shared location. Then, starting from that point, we can jointly optimize the star and galaxy parameters. If this cuts down on the number of Newton steps, it could be a big practical win. Also, when we start from peak inits, doing something like this will probably be sensible. Incidentally, another possibility would be just to give stars and galaxies their own location parameters, just like they have their own brightness parameters. Even with this, though, we may want to use a star fit to initialize the galaxy locations. Investigating this requires something like omitted ids. I'll look into it once you've restored the functionality one way or another. I'll leave it up to you how to prioritize.

…

On Mon, Dec 19, 2016 at 1:09 PM, Jeffrey Regier ***@***.***> wrote: @jrevels <https://github.com/jrevels> is work on refactoring Transform.jl now. I was thinking we'd get rid of omitted_ids and keep_ids because of the complexity, and because it doesn't seem as necessary any more. If we want to restore that functionality, the way to do it is probably to define another transform, like the box constraints or the simplex constraint that removes certain ids. Are you using omitted_ids often? Is restoring this functionality a high priority? (It would take resources away from other things.) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#480 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AESWPhDKW1FiL5KQyAH2tHd9X1rYkptMks5rJvKRgaJpZM4LREv6> .