Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FI new function for users #1822

Merged
merged 9 commits into from
Jul 26, 2022
Merged

FI new function for users #1822

merged 9 commits into from
Jul 26, 2022

Conversation

nirhutnik
Copy link
Contributor

@nirhutnik nirhutnik commented Jul 25, 2022

Closes #1757

Added function calculate_feature_importance in new file tabular/user_utils.py for users to use.
Changed the warnings for force_permutation=True so they will fail the process and we won't continue to built-in feature importance.
Added paragraph in feature_importance.rst.

NOTE!
Changed behavior of calculate_feature_importance in features.py - Now if force_permutation=True, and it fails to calculate permutation importance, the function raises an error (previous behavior skipped and tried model built-in importance, which is not intuitive and doesn't make sense IMO).

Added file user_utils.py
Created tests
Changed exceptions in feature importance calculation
@nirhutnik nirhutnik added documentation modification of the documentation / readme's feature Feature update or code change to the package labels Jul 25, 2022
@nirhutnik nirhutnik requested review from a team as code owners July 25, 2022 20:34
@nirhutnik nirhutnik self-assigned this Jul 25, 2022
deepchecks/tabular/user_utils.py Outdated Show resolved Hide resolved
tests/tabular/test_user_utils.py Outdated Show resolved Hide resolved
deepchecks/tabular/user_utils.py Outdated Show resolved Hide resolved
@JKL98ISR
Copy link
Member

can you give a better name for the pr?

@nirhutnik nirhutnik changed the title Added function FI new function for users Jul 26, 2022
@nirhutnik nirhutnik requested a review from a team as a code owner July 26, 2022 13:43
Copy link
Contributor

@Nadav-Barak Nadav-Barak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!
Additional suggestion: maybe show case this option in some places in docs (specifically plots that calculate FI multiple times), see comment in board issue

@nirhutnik nirhutnik enabled auto-merge (squash) July 26, 2022 19:06
@nirhutnik nirhutnik merged commit 90f0e92 into main Jul 26, 2022
@delete-merged-branch delete-merged-branch bot deleted the fi_functionality_1757 branch July 26, 2022 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation modification of the documentation / readme's feature Feature update or code change to the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] Enable explicit control of FI calculation
4 participants