FIX - computation of subdiff_distance in WeightedGroupL2 penalty
#225
Conversation
There was a problem hiding this comment.
Thank you so much for the quick fix @Badr-MOUFAD !
For future maintenance, maybe we could add details on the group Lasso subdifferential computation in the tutorial as well https://contrib.scikit-learn.org/skglm/tutorials/prox_nn_group_lasso.html#prox-nn-group-lasso
|
@Badr-MOUFAD @QB3 I'm done with the pass on docs, make any changes you want and merge |
|
@QB3 a glance at the derivation from you is a plus |
Badr-MOUFAD
changed the title
subdiff_distance in WeightedGroupL2 penaltysubdiff_distance in WeightedGroupL2 penalty
|
|
||
| .. math:: | ||
|
|
||
| D(v) = \min_{u \in \lambda \mathcal{B}_2, n \in \mathbb{R}_{-}^g} \ || u + n - v || |
There was a problem hiding this comment.
@Badr-MOUFAD this hides the problem, because why is the solution for a negative v, n = 0?
I'm subjective but I find it way clearer the way it was before, which avoids reference to an external source
There was a problem hiding this comment.
@Badr-MOUFAD this hides the problem, because why is the solution for a negative v, n = 0?
if v is negative, one can set n = v and u = 0 to get a distance equal zero (zero is a lower bound on the problem)
I'm subjective but I find it way clearer the way it was before, which avoids reference to an external source
The external source is a stack overflow thread to justify the computation of the min regardless of the order.
I believe the benefit of having formulas is to easily and efficiently look up the derivation later.
thought, I'm +1 for adding an explanation, that way we can have both worlds.
There was a problem hiding this comment.
@QB3 a glance at the derivation from you is a plus
I also think Mathurin's version was clearer. I would also try to be more self-contained (e.g. we currently talk about the distance, without always clearly stating that this is the distance of the gradient to the subdiff), but writing something clear and self-contained might take more time.
TLDR: I am happy to merge Mathurin's version and iterate later on the group lasso doc
Context of the PR
This fixes the bug reported in issue #197 (thanks for the investigation @tomaszkacprzak)
Contributions of the PR
subdiff_distanceChecks before merging PR
[ ] added documentation for any new feature[ ] edited the what's new