raleng / nmf

Non-negative matrix factorization using MUR, ANLS, ADMM or AO-ADMM.
Other
10 stars 2 forks source link

admm_ls_update function is not getting invoked by the admm module #4

Closed dguhanus closed 1 year ago

dguhanus commented 1 year ago

Dear Ralf Engbers, I have noticed one logical error in the NMF ADMM module. You are not invoking the function of admm_ls_update or admm_kl_update in the admm module. But you are invoking them in other method like AO-ADMM. Here is the code snippet: ` for i in range(max_iter):

    if distance_type == 'eu':
        h_aux = aux_update(h, dual_h, w_aux, v, None, rho, distance_type)
        w_aux = aux_update(w.T, dual_w.T, h_aux.T, v.T, None, rho, distance_type)
        w_aux = w_aux.T

        h = prox(reg_h[1], h_aux, dual_h, rho=rho, lambda_=reg_h[0])
        w = prox(reg_w[1], w_aux.T, dual_w.T, rho=rho, lambda_=reg_w[0])
        w = w.T

` I think this might be a logical error. Please correct me if I am wrong.

raleng commented 1 year ago

I haven't worked on or with these algorithms in years, so I am a bit loose on the details, but this is how I remember it.

ADMM works by

  1. splitting the problem into two (here h and w),
  2. fixing one variable solving for the other, and then
  3. fixing the other variable and solving for the first.

AO-ADMM on the other hand is a hybrid algorithm, that actually tries to optimize the variables in between updates, hence calling the loss function updates, admm_ls_update and admm_kl_update respectively, on each update.

So I think this is not a logical error, but rather confusing code organization (including the optimization functions in the admm module without using them) and sub-optimal naming (admm_XX_update does give the impression it is used for the ADMM part, not for the AO part).

Again, haven't worked on this in years, so please fact check this and make sure this makes sense.