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

adding feature_threshold replacement for show_all_features #1097

Merged
merged 11 commits into from Aug 25, 2020

Conversation

bchen1116
Copy link
Contributor

@bchen1116 bchen1116 commented Aug 24, 2020

fix #869

Replace show_all_features parameter in graph_permutation_importance() and graph_feature_importance() functions

Creating parameter feature_threshold to replace show_all_features for graphing feature importance.

@bchen1116 bchen1116 self-assigned this Aug 24, 2020
@codecov
Copy link

codecov bot commented Aug 24, 2020

Codecov Report

Merging #1097 into main will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1097   +/-   ##
=======================================
  Coverage   99.91%   99.91%           
=======================================
  Files         192      192           
  Lines       10719    10736   +17     
=======================================
+ Hits        10710    10727   +17     
  Misses          9        9           
Impacted Files Coverage Δ
evalml/model_understanding/graphs.py 100.00% <100.00%> (ø)
evalml/pipelines/pipeline_base.py 100.00% <100.00%> (ø)
...lml/tests/model_understanding_tests/test_graphs.py 100.00% <100.00%> (ø)
evalml/tests/pipeline_tests/test_graphs.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30cc109...d8f686c. Read the comment docs.

@bchen1116 bchen1116 marked this pull request as ready for review Aug 24, 2020
@bchen1116 bchen1116 requested review from dsherry and angela97lin Aug 25, 2020
Copy link
Collaborator

@dsherry dsherry left a comment

@bchen1116 great work!! Nice unit tests.

I have a few requests before you merge:

  • I left a suggestion about renaming the parameter to importance_threshold
  • Remove unnecessary threshold values from a few of the tests, or explain why they're necessary
  • Please see my comment about updating the two new unit tests you've added.
  • Delete the file evalml/tests/utils_tests/test_graph_utils.py which was added accidentally (was recently deleted on main)

I left some other nit-pick comments on style and wording, but those aren't blocking merge and could be addressed in a separate PR if you prefer.

evalml/model_understanding/graphs.py Outdated Show resolved Hide resolved
evalml/model_understanding/graphs.py Outdated Show resolved Hide resolved
evalml/model_understanding/graphs.py Outdated Show resolved Hide resolved
evalml/model_understanding/graphs.py Outdated Show resolved Hide resolved
evalml/pipelines/pipeline_base.py Outdated Show resolved Hide resolved
evalml/tests/model_understanding_tests/test_graphs.py Outdated Show resolved Hide resolved
evalml/tests/model_understanding_tests/test_graphs.py Outdated Show resolved Hide resolved
Copy link
Contributor

@angela97lin angela97lin left a comment

I think Dylan covered the major points but looks good after those comments are addressed!

evalml/tests/pipeline_tests/test_graphs.py Outdated Show resolved Hide resolved
@bchen1116 bchen1116 requested review from dsherry and angela97lin Aug 25, 2020
@bchen1116 bchen1116 merged commit b7442cf into main Aug 25, 2020
@dsherry dsherry mentioned this pull request Aug 25, 2020
@bchen1116 bchen1116 deleted the bc_869_feature_importance branch Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change show_all_features parameter to threshold for feature importance graphs
3 participants