ENH Add positive constraint to L1, WeightedL1 and L1_plus_L2
#110
Conversation
|
So far, it's working with |
|
Anyone to discuss with me about the impact of the positive constraint on the distance to the subdifferential? I try to do the maths, but I'm stuck... (poke @mathurinm @Klopfe @QB3 @Badr-MOUFAD ) |
|
For L1 + positivity constraint, when x is strictly negative, the penalty is + infty and the subdifferential is empty, so distance is infinite. When x is 0, the subdiff is ]-infty,1]. When x is > 0, the subdiff is the singleton 1 |
|
Note to self: do same for L21, docstrings and implem might need compression |
There was a problem hiding this comment.
hats off @PABannier for this work!
Some remarks and some suggestions ⬇️
| if w[j] == 0: | ||
| # distance of - grad_j to [-alpha, alpha] | ||
| subdiff_dist[idx] = max(0, np.abs(grad[idx]) - self.alpha) | ||
| if self.positive: |
There was a problem hiding this comment.
Suggestion that I think would enhance the readability of this part
# case of positivity constraints on coefficients
if positive:
# loop over features to compute subdiff distance
...
...
return subdiff distance
# usual case
...
....
...WDTY? (@mathurinm any thoughts?)
There was a problem hiding this comment.
i find it redundant. I think it's already quite readable. @mathurinm ?
There was a problem hiding this comment.
Apart from that, anything else? (any suggestions @mathurinm ?)
|
@mathurinm @Badr-MOUFAD should be mergeable |
| subdiff_dist[idx] = max(0, - grad[idx] - self.alpha * self.l1_ratio) | ||
| else: | ||
| # distance of -grad_j to alpha * {l1_ratio + (1 - l1_ratio) * w[j]} | ||
| subdiff_dist[idx] = np.abs( |
There was a problem hiding this comment.
all is good for me, except for this line (the one below also): it's difficult to read as it spans multiple lines.
Any suggestions @PABannier to increase its readability?
There was a problem hiding this comment.
I don't find it particularly hard to read. But one thing that could be improved is to factor the alpha for less redundancy.
There was a problem hiding this comment.
LGTM! Many thanks @PABannier.
We should consider adding this feat to Lasso, WeightedLasso and ElasticNet estimators
Add positivity constraints to these penalties and corresponding estimators.
Question: do we want it enabled for
WeightedL1? It seems more consistent and is not tricky to do.Closes #107, #132