GH-16807: enable remove_offset_effects with cross-validation in GLM - #16838
Open
maurever wants to merge 25 commits into
Open
GH-16807: enable remove_offset_effects with cross-validation in GLM#16838maurever wants to merge 25 commits into
maurever wants to merge 25 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Enables remove_offset_effects to be used together with GLM cross-validation by updating CV fold scoring behavior and ensuring scoring history correctly surfaces CV deviance columns for restricted vs unrestricted (with-offset) views.
Changes:
- Removes the validation restriction that blocked
remove_offset_effectswhennfolds > 0. - Updates GLM CV fold scoring + scoring history generation to compute and display restricted vs unrestricted CV deviance (
deviance_xval,deviance_se) independently. - Adds/updates Java, Python, and R regression tests covering training success, metric differences, and scoring history columns.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| h2o-algos/src/main/java/hex/glm/GLMModel.java | Allows remove_offset_effects with CV by removing the validation error. |
| h2o-algos/src/main/java/hex/glm/GLM.java | Forces RO semantics during CV holdout scoring; computes separate restricted/unrestricted CV deviance arrays for scoring history. |
| h2o-algos/src/main/java/hex/glm/GLMUtils.java | Prefixes unrestricted deviance_xval/deviance_se columns when combining restricted/unrestricted scoring histories. |
| h2o-algos/src/test/java/hex/glm/GLMControlVariablesAndRemoveOffsetTest.java | Updates existing CV test to expect success; adds regression + scoring-history assertions. |
| h2o-py/tests/testdir_algos/glm/pyunit_GH_16807_glm_remove_offset_cv.py | Adds Python regression test for RO+CV training, deviance differences, and scoring history columns. |
| h2o-r/tests/testdir_algos/glm/runit_GH_16807_GLM_remove_offset_cv.R | Adds R regression test for RO+CV training and scoring history deviance_xval/deviance_se. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- GLMModel.java: remove validation block that rejected remove_offset_effects+CV combination - GLM.java: pass _parms._remove_offset_effects to GLMResDevTask in CV fold scoring so holdout deviances are computed without the offset contribution - GLM.java: compute separate unrestricted (with-offset) xval deviance arrays for generate_scoring_history=True, keeping restricted and unrestricted histories independent - GLM.java: pass xval deviance arrays into restricted scoring history to2dTable calls so deviance_xval and deviance_se appear correctly when nfolds>0 - GLMUtils.java: prefix deviance_xval and deviance_se with "Unrestricted " in combineScoringHistoryRestricted, consistent with existing objective/NLL columns - GLMControlVariablesAndRemoveOffsetTest: update testRemoveOffsetWithCrossValidation to expect success; add regression guard, beta invariance, and scoring history tests - pyunit_GH_16807_glm_remove_offset_cv.py: new Python test - runit_GH_16807_GLM_remove_offset_cv.R: new R test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
maurever
force-pushed
the
maurever_GH-16807_remove_offset_effects_cross_validation
branch
from
May 11, 2026 07:47
19b58cb to
64c8dac
Compare
tomasfryda
reviewed
May 29, 2026
tomasfryda
reviewed
May 29, 2026
- Fix sparseOffset zeroed incorrectly in GLMResDevTask when
remove_offset_effects=true (standardization correction, unrelated
to the offset column)
- Fix memory leak: add remove_impl to GLMModel to clean up unrestricted
training/validation metrics from DKV on model delete
- Add DKV.put after buildMainModel in ModelBuilder to prevent cache
miss before cv_mainModelScores
- Remove IcedUtils.deepCopy and dead dinfo.clone() from
MakeGLMModelHandler; fix validation to three independent guards
- Move all glm.py/glm.R changes to generators (gen_glm.py, gen_glm.R);
add cross_validation_metrics_unrestricted_model property; fix
make_derived_glm_model validation
- Update GLMV3.java @API help text for remove_offset_effects and
control_variables (CV support, constraints)
- Fix remove_offset_effects.rst example to use public property instead
of _model_json
- Add P0-1 tests: make_derived_glm_model with remove_offset_effects
only (with and without CV)
tomasfryda
requested changes
Jun 15, 2026
tomasfryda
left a comment
Contributor
There was a problem hiding this comment.
runit_GLM_make_derived_model.R:
══ Failed tests ═════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
── Error: GLM: Test make derived model ─────────────────────────────────────────
Error: Failed to get error in evaluating h2o.make_derived_glm_model(pro ...
Backtrace:
▆
1. ├─global withWarnings(test())
2. │ └─base::withCallingHandlers(expr, warning = wHandler)
3. └─global test()
4. └─tools::assertError(...)
5. └─base::tryCatch(...)
6. └─base (local) tryCatchList(expr, classes, parentenv, handlers)
7. └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
8. └─value[[3L]](cond)
[ FAIL 1 | WARN 0 | SKIP 0 | PASS 3 ]
######## ### #### ##
## ## ## ## ##
## ## ## ## ##
###### ## ## ## ##
## ######### ## ##
## ## ## ## ##
## ## ## #### ########
[2026-06-15 10:03:10.19476] [ERROR] : Error: Failures detected.
[2026-06-15 10:03:10.19502] [ERROR] : TEST FAILED
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…_validation_predictions=true - GLM.cv_mainModelScores: when keep_cross_validation_predictions=true, store per-fold unrestricted prediction keys in output._cross_validation_predictions_unrestricted_model and untrack frames from scope (mirrors super's restricted handling). Otherwise delete them after combining, as before. - GLMOutput: add _cross_validation_predictions_unrestricted_model field (Key<Frame>[]). - GLMModel.deleteCrossValidationPreds: remove per-fold unrestricted frames on model delete. - GLMModelV3: add schema field cross_validation_predictions_unrestricted_model. - Tests: assert per-fold array is populated (with correct length) when keep_cross_validation_predictions=true and null otherwise. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Null out the source model's _cross_validation_holdout_predictions_frame_id_unrestricted_model after creating the derived model so the source's remove_impl does not delete a frame now owned by the derived model. Guard: throw IllegalArgumentException if the field is already null when make_derived_model is called — the frame was consumed by a prior derivation.
…viance fix - make_derived_glm_model: deep-copy the CV holdout-predictions frame into a derived-owned key instead of transferring/nulling the source's pointer, so deleting a derived model no longer blocks re-deriving from the source; restore a real provenance check on an occupied dest key instead of blind-casting or silently returning whatever is there - Fix raw-beta denormalization bug in make_derived_model/make_unrestricted_model: clone the source DataInfo and reset its transform to NONE instead of mutating the shared source instance, so isStandardized() and beta(lambda) no longer misreport/corrupt an already-raw beta. Leave the same pre-existing bug in make_model documented but unfixed — that endpoint predates and is unrelated to the derived-model feature this change targets - GLMMetricBuilder: zero the offset before computing null_deviance under _useRemoveOffsetEffects, fixing the Model.BigScore-based CV regression scoring path where null_deviance mixed with-/without-offset quantities - GLMModel.deleteCrossValidationPreds: drop the redundant/asymmetric removal of the unrestricted CV metric (already covered by remove_impl's _model_metrics cleanup); the metric must survive when this method runs standalone (e.g. StackedEnsemble base-learner cleanup) on a live model - Set the unrestricted CV metric's _description before addModelMetrics() DKV.puts it, so the description is actually persisted rather than only updated on the in-memory object - ModelBuilder: guard DKV.put(_result.get()) against a null model - Fix broken PSA doctest in make_derived_glm_model examples - Add Python cross_validation_metrics_summary_unrestricted_model accessor, correct the cross_validation_metrics_unrestricted_model docstring type, and move test-only TwoDimTable.findRowIndex into the test file - Add test coverage for the Poisson CV+offset scoring path, the restricted/unrestricted same-seed invariant, fold_column-based CV, the make_derived_model provenance/re-derive behavior, and the metric survival/description-persistence regressions above
… derived-model holdout-preds propagation - GLM.java: reject control_variables combined with multinomial/ordinal family (mirrors the existing remove_offset_effects + family guard; the old _distribution-based check in GLMModel.java never fires for GLM's own _family enum, so this combination previously trained unvalidated) - remove_offset_effects.rst: fix broken CV example calling residual_deviance(xval=True), which always raises; use model_performance(xval=True).residual_deviance() instead - MakeGLMModelHandler.java: factor the CV holdout-predictions deep-copy into a shared copyHoldoutPreds helper and call it from the remove_offset_effects branch of make_derived_model, which previously left the derived model's holdout-predictions frame null even when keep_cross_validation_predictions was set (only the unrestricted branch copied it before) - Add regression tests: holdout-preds propagation for the remove_offset_effects derive branch, and Gaussian + standardize=True + weights_column under CV (previously only binomial/Poisson were exercised with remove_offset_effects+CV) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
tomasfryda
reviewed
Jul 13, 2026
tomasfryda
reviewed
Jul 13, 2026
tomasfryda
reviewed
Jul 13, 2026
|
tomasfryda
approved these changes
Jul 14, 2026
tomasfryda
left a comment
Contributor
There was a problem hiding this comment.
LGTM, thank you @maurever !
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
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.


Issue: #16807