tsai icon indicating copy to clipboard operation
tsai copied to clipboard

learn.feature_importance does not work when list of feaures names provided

Open idanre1 opened this issue 3 years ago • 1 comments

After training when trying to pass a list as features names listify changes the len of feature_names to 1 causes bogus assertion error. Bug is at analysis.py line 187 (latest codebase in github)

Code:

def feature_importance(self:Learner,
...
...
...
    # Selected vars & feature names
    sel_vars = not(isinstance(self.dls.sel_vars, slice) and self.dls.sel_vars == slice(None, None, None))
    if feature_names is None:
        feature_names = L([f"var_{i}" for i in range(X.shape[1])])
        if sel_vars:
            feature_names = feature_names[self.dls.sel_vars]
    else:
        pass # feature_names = listify(feature_names)   <----- Bug

    if sel_vars:
        assert len(feature_names) == len(self.dls.sel_vars)
    else:
        assert len(feature_names) == X.shape[1] #<------- Bogus assertion

I would suggest remove the listify since len(feature_names) will always be 1

idanre1 avatar Apr 27 '22 08:04 idanre1

Hi @idanre1, First of all, sorry for my late reply. I don't understand this issue. If you have a list of variable names and pass it through listify, the length of the list won't change. Please, try this:

feat_names = [f'feat_{i}' for i in range(10)]
len(feat_names), len(listify(L(feat_names)))

output: 10, 10 Am I missing anything here?

oguiza avatar May 13 '22 07:05 oguiza

Works without problems for me:

paulbauriegel avatar Aug 14 '22 22:08 paulbauriegel

Hi @idanre1, Could you please confirm if this issue is still relevant? Based on @paulbauriegel 's comment (and my experience) it seems to be working well.

oguiza avatar Aug 18 '22 09:08 oguiza

Hi Thank you guys for the support. @paulbauriegel by diff your code I think I can refine the issue. image

I am passing listify pandas index and not a list. I think I need to post this issue at fastai repo and not in tsai.

Thanks! Idan

idanre1 avatar Aug 22 '22 17:08 idanre1