FIX : broken MultiTaskLasso with warm_start=True #12853
Conversation
| @@ -1796,7 +1796,7 @@ def fit(self, X, y): | |||
| X, y, X_offset, y_offset, X_scale = _preprocess_data( | |||
| X, y, self.fit_intercept, self.normalize, copy=False) | |||
|
|
|||
| if not self.warm_start or self.coef_ is None: | |||
There was a problem hiding this comment.
we were trying to access a fit param before actually setting it.
|
Are we sure we rather not allow |
|
@jnothman what do you initially? what is sure is that now the code breaks out of the box if you call fit with warm_start=True |
| @@ -1796,7 +1796,7 @@ def fit(self, X, y): | |||
| X, y, X_offset, y_offset, X_scale = _preprocess_data( | |||
| X, y, self.fit_intercept, self.normalize, copy=False) | |||
|
|
|||
| if not self.warm_start or self.coef_ is None: | |||
| if not self.warm_start or not hasattr(self, "coef_"): | |||
There was a problem hiding this comment.
@jnothman shall I replace not hasattr(self, "coef_") by a getattr and we're good?
There was a problem hiding this comment.
I'm okay with it either way, actually. I just never know how flexible we should make these kinds of changes... I mean if someone's gone and changed their code now to set coef_ = None so that it works, using getattr would make sure it continues to work in the next release... but it's also hacky.
|
As @qinhanmin2014 suggested on gitter I think we need a new 0.20.3 section in the changelog for this fix. |
|
+1 also for backporting #12361 to 0.20.X |
There's already an entry in 0.21, I'm not sure whether we should have two entries or move the entry to 0.20.3.
I prefer the test in #12361, I think it'll be better to keep consistency between master and 0.20.X (if it's possible) |
|
this has been closed without being fixed. the test did I miss something @jnothman ? |
|
@agramfort 0.20.3 in not released. We need to backport #12361 and #12984 to 0.20.X and release 0.20.3 (both PRs are tagged 0.20.3) |
|
ok I had missed that sorry. It was not referenced in the PR apparently. |
|
Yes, I think we could do a 0.20.3 release and worry about remaining ARM
issues later...?
|
Related to #12844
warm_start=True is currently broken in 0.20.X branch but not in master. I'll send a PR to master now to make sure we don't break this in the future.