GH-16757: Fix top_n_features in shap_summary_plot in R#16790
GH-16757: Fix top_n_features in shap_summary_plot in R#16790tomasfryda wants to merge 1 commit intorel-3.46.0from
Conversation
|
There was a problem hiding this comment.
Pull request overview
Fixes an off-by-one issue in the R h2o.shap_summary_plot feature-selection logic so top_n_features renders exactly the requested number of features, and adds regression tests in both R and Python.
Changes:
- Fix
top_n_featuresslicing inh2o.shap_summary_plot(R) and drop unused factor levels before selecting features. - Add an R unit test covering typical and corner-case
top_n_featuresvalues (including0erroring and-1meaning “all”). - Add a Python unit test intended to validate
top_n_featuresbehavior via plotted y-axis labels.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| h2o-r/tests/testdir_misc/explain/runit_shap_summary_plot_top_n_features.R | Adds R regression tests for top_n_features sizing and corner cases. |
| h2o-r/h2o-package/R/explain.R | Fixes off-by-one feature selection and adds top_n_features == 0 validation. |
| h2o-py/tests/testdir_misc/explain/pyunit_shap_summary_plot_top_n_features.py | Adds Python regression test that inspects rendered tick labels to count displayed features. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ytick_labels_default = [t.get_text() for t in ax_default.get_yticklabels()] | ||
| n_default = len([l for l in ytick_labels_default if l.strip()]) | ||
| matplotlib.pyplot.close() | ||
| assert n_default <= 20, f"Default top_n_features=20 showed {n_default} features" |
There was a problem hiding this comment.
Is it possible that the n_default is less than 20? Shouldn't it be exactly 20? Also, please save the value 20 as a variable defined from data.
| assert n_default <= 20, f"Default top_n_features=20 showed {n_default} features" | |
| assert n_default == 20, f"Default top_n_features=20 showed {n_default} features" |
There was a problem hiding this comment.
Is it possible that the n_default is less than 20?
yes
Shouldn't it be exactly 20?
no
Also, please save the value 20 as a variable defined from data.
This limit is not data dependent.
#16757