Fix GAM splines_non_negative missing from actual_params by correcting boolean[] parameter serialization#16862
splines_non_negative missing from actual_params by correcting boolean[] parameter serialization#16862Conversation
splines_non_negative missing from actual_params by correcting boolean[] parameter serialization
|
@copilot , changes look good. Could you add a test where the actual parameters in the GAM algorithm are correct? Especially, the |
Added in commit I introduced a focused GAM regression test at |
|
Agent-Logs-Url: https://github.com/h2oai/h2o-3/sessions/d72e9833-8f8f-4d4d-8ff0-ccb246c7aca5 Co-authored-by: maurever <11465784+maurever@users.noreply.github.com>
Agent-Logs-Url: https://github.com/h2oai/h2o-3/sessions/151d6bd7-b294-4cd5-af22-60879930fe75 Co-authored-by: maurever <11465784+maurever@users.noreply.github.com>
8776e30 to
8f8b6fb
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect JSON serialization of boolean[] values in water.IcedWrapper, which caused certain model parameters (notably GAM’s splines_non_negative) to appear as None/missing in actual_params despite being explicitly set. The change improves parameter metadata fidelity for clients that rely on IcedWrapper.writeUnwrappedJSON for polymorphic actual_value serialization.
Changes:
- Add first-class
boolean[]support towater.IcedWrapper(store, retrieve viaget(), stringify, and serialize as a JSON boolean array). - Add JUnit coverage validating
boolean[]round-trip viaget()and exact JSON output. - Add a Python regression test ensuring GAM preserves
splines_non_negativein bothactual_paramsandparms[..]["actual_value"].
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| h2o-core/src/main/java/water/IcedWrapper.java | Implements boolean[] wrapping and correct unwrapped JSON serialization ([true,false]), preventing loss of boolean-array parameter values in model metadata. |
| h2o-core/src/test/java/water/IcedWrapperTest.java | Adds targeted unit tests for boolean[] retrieval and JSON output to prevent regressions in core serialization behavior. |
| h2o-py/tests/testdir_algos/gam/pyunit_gam_splines_non_negative_actual_params.py | Adds an end-to-end GAM regression test validating client-visible parameter preservation in actual_params and parameter schema metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tomasfryda
left a comment
There was a problem hiding this comment.
LGTM, thank you @maurever !
|
Thank you for the fix! Suggestion: reuse the existing The fix is correct, but Every other array case/if-else in
// AutoBuffer.java:1791
AutoBuffer putJSONAZ(boolean[] b) { // was: privateThen in else if (t.equals("Bo"))
return ab.putJSONAZ(b_ar);Not a blocker — the current code works — just a maintainability/consistency cleanup. (Drafted with Claude Code.) |
IcedWrapper.writeUnwrappedJSON duplicated logic already present in AutoBuffer.putJSONAZ(boolean[]). Widen its visibility to package-private (matching putJSONA4/putJSONA8/putJSONA4f). Co-authored-by: Claude <noreply@anthropic.com>
|
|
@tomasfryda @valenad1, could you please review again? Thank you. |



GH Issue: #16602
GAMmodels were returningsplines_non_negativeasNoneinactual_paramseven when explicitly set. The issue was in core parameter JSON serialization:boolean[]values were wrapped but emitted asnull.Root cause
water.IcedWrapperdid not correctly supportboolean[]in JSON output (writeUnwrappedJSONreturnednullfor boolean arrays).actual_valueserialization, sosplines_non_negativewas dropped toNonein clients.Changes made
h2o-core/src/main/java/water/IcedWrapper.javaboolean[]support inIcedWrapper:b_ar)get()toString()[true,false,...]) instead ofnullh2o-core/src/test/java/water/IcedWrapperTest.javaboolean[]boolean[]h2o-py/tests/testdir_algos/gam/pyunit_gam_splines_non_negative_actual_params.pysplines_non_negative=[True, False]and verifies the value is preserved in:model.actual_params["splines_non_negative"]model.parms["splines_non_negative"]["actual_value"]Illustrative behavior