Repository navigation
data: fix PartialDependence crash on datasets with fewer than 10 rows - #682
Closed
aniruddhaadak80 wants to merge 1 commit into
Closed
aniruddhaadak80 wants to merge 1 commit into
aniruddhaadak80 wants to merge 1 commit into
Conversation
…etml#681) _gen_pdp built an individual conditional expectation line for every row of the input data and then kept a random subsample of them as "background_scores" for plotting: ice_lines = ice_lines[ np.random.choice(ice_lines.shape[0], num_ice_samples, replace=False), : ] num_ice_samples defaults to 10 and is not exposed on the PartialDependence constructor. Because the subsample is drawn with replace=False, NumPy refuses to draw more items than the population size, so every dataset with fewer than num_ice_samples rows raised "ValueError: Cannot take a larger sample than population when 'replace=False'" instead of producing an explanation. The underlying data size was never checked or surfaced to the caller. Clamp the requested count to the number of available rows so smaller datasets yield one background line per row rather than crashing. Datasets at or above num_ice_samples are unaffected, since the min() resolves back to the original value. The only consumer of background_scores is interpret/visual/plot.py, which iterates it by row ("for i in range(background_lines.shape[0])"), so a smaller subsample renders correctly with no other change. Add tests/blackbox/test_partialdependence.py with a parametrized case over 1/2/5/9 rows, an end-to-end PartialDependence construction on a 3-row dataset, and a case asserting that a 25-row dataset still keeps exactly num_ice_samples background lines. Fixes interpretml#681
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #682 +/- ##
===========================================
- Coverage 67.21% 22.23% -44.98%
===========================================
Files 77 77
Lines 11735 11736 +1
===========================================
- Hits 7888 2610 -5278
- Misses 3847 9126 +5279 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Author
|
Superseded: this commit is missing the DCO \Signed-off-by:\ trailer that this repo requires. Since the branch must not be force-pushed, the same fix is resubmitted as a fresh PR from a correctly signed-off commit. Closing to avoid duplicate review effort. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
interpret.blackbox.PartialDependenceraisedValueError: Cannot take a larger sample than population when 'replace=False'for any dataset with fewer than 10 rows, because the ICE background subsample was drawn without replacement using a hardcoded default of 10. This clamps the requested sample count to the number of available rows.Root cause
In
python/interpret-core/interpret/blackbox/_partialdependence.py,_gen_pdpcomputes one individual conditional expectation line per input row, then keeps a random subsample of them as thebackground_scoresseries used for plotting:num_ice_samplesdefaults to10and_gen_pdpis only called fromPartialDependence.__init__(line 119), which does not thread this parameter through, so every caller gets 10. Because the draw usesreplace=False, NumPy refuses to select more items than the population size, so any dataset with fewer than 10 rows raised. Nothing in__init__validated or surfaced the data size, and the failure surfaced as a raw NumPy error from deep inside the explainer.The only consumer of
background_scoresisinterpret/visual/plot.py:390, which iterates it by row (for i in range(background_lines.shape[0])), so returning fewer lines than requested is handled correctly and needs no downstream change.Changes
python/interpret-core/interpret/blackbox/_partialdependence.py— clampnum_ice_samplestoice_lines.shape[0]before thenp.random.choicedraw in_gen_pdp. Datasets at or above the threshold are unaffected becausemin()resolves back to the original value.python/interpret-core/tests/blackbox/test_partialdependence.py— new regression tests: a parametrized case over 1/2/5/9 rows asserting the crash is gone and that exactly one background line per row is returned, an end-to-endPartialDependence+explain_global()construction on a 3-row dataset, and a 25-row case asserting the existing cap ofnum_ice_samplesstill holds.Testing
Run from
python/interpret-corewithPYTHONPATHpointed at that directory.Before the fix — new tests fail, existing behaviour for large data already passes:
After the fix — all 6 pass:
Existing blackbox test suite (including the pre-existing
test_sensitivity.py) is green:Formatting matches the
ruff-formathook configured in.pre-commit-config.yaml:ruff checkis clean on the new test file. On the modified_partialdependence.pyit reportsI001(import sorting),NPY002(legacynp.random.choice) andPLC0415(deferred import) — all three are pre-existing onmainand unchanged by this PR, and this repo's pre-commit only runsruff-format, so they are left alone.Fixes #681