Repository navigation
Fix merge_ebms crash when every feature is categorical - #701
flyingdoubleG wants to merge 1 commit into
Conversation
merge_ebms only set feature_bounds_ on the merged model when some bound was not NaN. With only categorical features every bound is NaN, so the attribute was never set and the later _harmonize_tensor calls raised AttributeError. Fitted EBMs keep NaN bounds for categoricals, so do the same here. Signed-off-by: flyingdoubleG <53119724+flyingdoubleG@users.noreply.github.com>
rahul05ranjan
left a comment
There was a problem hiding this comment.
Traced the fix against the PR branch — the premise and crash chain check out:
- A fitted EBM always sets
feature_bounds_(glassbox/_ebm.py:1759), and the documented semantics state categorical features have min/maxNaN(docs at _ebm.py:3982-3984). So an all-categorical fitted model has the attribute with all-NaNrows. - Pre-fix, the
any(not isnan(val) ...)guard skipped the assignment when every feature is categorical in every model, so the merged EBM came out withoutfeature_bounds_at all. The attribute is then read directly at the three_harmonize_tensorcall sites in_merge_ebms.py(lines 698, 737, 750), so the merge crashes withAttributeErrorbefore producing a model — matching the issue title. - Post-fix, the merged model matches the fitted-model contract (
np.isnan(merged.feature_bounds_).all()), and the new test also pins the bag-weighted prediction average, which is a nice invariant to lock in.
I tried to reproduce end-to-end on this machine but the current interpret-core wheel ships a 32-bit libebm.so, so the native core won't load on arm64 and the verification is static. The test itself looks faithful to the crash scenario (two categorical-only regressors with different bag counts).
One thing the diff doesn't change that might be worth a look in a follow-up: when none of the input models expose feature_bounds_ (e.g. the DP path), len(min_feature_vals) == 0 still skips the assignment, so the merged model can still lack the attribute — that's the pre-existing contract, just noting it's now the only remaining way to hit it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #701 +/- ##
==========================================
- Coverage 67.80% 67.76% -0.04%
==========================================
Files 77 77
Lines 11893 11892 -1
==========================================
- Hits 8064 8059 -5
- Misses 3829 3833 +4 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:
|
merge_ebmscrashes when every feature in the models is categorical:A fitted EBM always has
feature_bounds_, with NaN rows for categorical features.merge_ebmscombines the input bounds withnanmin/nanmax, but it only assignsebm.feature_bounds_when at least one merged bound is not NaN. With only categorical features every bound is NaN, so the attribute is never set. The_harmonize_tensorcalls further down then readebm.feature_bounds_and raise.The fix always assigns the merged bounds, keeping the NaN rows the same way
fitdoes._harmonize_tensoronly reads the bounds for continuous features, so the NaN rows aren't used anywhere. Models with at least one continuous feature get exactly the same result as before.test_merge_ebms_all_categoricalmerges two regressors trained on disjoint halves of an all-categorical dataset, with a categorical pair term. It checks that the merged bounds are NaN and that the merged model predicts the bag-weighted average of the two input models,(3 * ebm1 + 5 * ebm2) / 8for 3 and 5 outer bags.Testing (Windows, Python 3.14, running the checkout with the prebuilt libebm from the 0.7.8 wheel, since I can't build libebm here):
tests/glassbox/ebm/test_merge_ebms.py: 10 passed. With main's_merge_ebms.py, the new test fails with theAttributeErrorabove.tests/glassbox/ebm/test_ebm.py -m "not slow and not selenium": 244 passed, 44 skipped, 1 failed. The failure istest_quantile_regression("Unrecognized objective type"), because the quantile objective is newer than the 0.7.8 library. It is unrelated to this change.ruff format --checkpasses on both files.AI usage: I used Claude Code. It found this while cross-checking
merge_ebmsagainst bag-weighted averages of the input models, then wrote the fix and the test. I reviewed the change.