search

fdtomasi / regain

REGAIN (Regularised Graphical Inference)
BSD 3-Clause "New" or "Revised" License
28 stars 13 forks source link

Parameter `over_relax` is never used in some classes. #9

Closed hrayrhar closed 5 years ago

hrayrhar commented 5 years ago

I have noticed the parameter over_relax in classes LatentGraphLasso, LatentTimeGraphLasso, and TimeGraphLasso cannot be changed. It will always be 1.0 (as specified in ancestor class GraphLasso). Is this a bug ?

fdtomasi commented 5 years ago

I would say this is more of a feature of GraphLasso rather than a bug of the others, which gives him a bit more flexibility.

This parameter comes from the first of our implementations of GraphLasso via ADMM inspired by this Matlab implementation. Note that the over_relax parameter is used "not to believe completely" to the prox operator in the z-step.

However, we actually never used it, so we did not propagate its implementation on the other classes. I guess its addition would complicate the code without any practical relevance, and the benefit of having an over-relaxation parameter on the soft-thresholding would not be enough compared to the additional effort one should use for cross-validate one more hyperparameter (which, for LatentGraphLasso and others) is already high.

However, if you see a practical relevance of having the parameter implemented also for the LatentGraphLasso (for start), we may discuss about implementing it for the other classes as well, and in that case I would be more than happy to accept a pull request about that if you want to contribute.

hrayrhar commented 5 years ago

Thanks @fdtomasi for the clarification.