Fix regression in RandomForestClassifier binary classification predictions

[Copilot]

Description

Restores the weights_are_all_positive_ flag in TreeEnsembleClassifier that was removed in PR #27552.

This flag controls how binary classification scores are converted to two-class probabilities in _set_score_binary:

• All-positive weights (RandomForest, where leaf weights are probabilities ∈ [0,1]): threshold > 0.5, complement via 1 - score (write_additional_scores = 0/1)
• Mixed weights (gradient boosted trees, where weights can be negative): threshold > 0, complement via -score (write_additional_scores = 2/3)

Without this distinction, RandomForest models produce negative "probabilities" and incorrect labels when the score falls in (0, 0.5).

The LOGISTIC post_transform path (the #27533 fix) is unaffected because write_scores applies sigmoid(score) / sigmoid(-score) identically for both cases 0/1 and 2/3.

Motivation and Context

PR #27552 fixed #27533 (LOGISTIC transform with negative weights) but inadvertently broke all binary TreeEnsembleClassifier models with non-negative weights and non-LOGISTIC post_transform — notably sklearn RandomForestClassifier conversions via skl2onnx.

# Reproducer: predictions no longer match sklearn, probabilities go negative
sess = ort.InferenceSession(onnx_model.SerializeToString())
onnx_labels, onnx_probs = sess.run(None, {"float_input": X_test})
# onnx_probs contains negative values, row sums ≠ 1.0

[Tim Head (betatim)]

Copilot can you add a non regression test (it should fail on main and pass with this PR applied)? This way we make sure the problem is properly fixed and won't come back

[Tianlei Wu (tianleiwu)]

The fix correctly restores the weights_are_all_positive_ distinction so binary classifiers with non-negative leaf weights (RandomForest-style) use the > 0.5 threshold with the 1 - score complement (codes 0/1), while mixed/negative-weight models keep > 0 with -score (codes 2/3). This matches the add_second_class contract in write_scores, and the LOGISTIC path is unaffected. Index access in the new loop is safe given the size invariants enforced in the base Init, and the aggregator constructor's member-init order matches declaration order.

One consistency suggestion is left inline. Otherwise this is a well-scoped regression fix with targeted tests covering the < 0.5, > 0.5, and 0.5-boundary cases.

[Hariharan Seshadri (hariharans29)]

Review — PR #28685 (Fix regression in RandomForestClassifier binary classification)

Verdict: approve. This is a clean regression fix for issue #28557 (sklearn RandomForestClassifier producing negative "probabilities" since ORT 1.26.0). It restores a flag that PR #27552 inadvertently dropped while fixing #27533 (LOGISTIC + non-negative weights). Both bugs can now coexist correctly.

What it fixes

PR #27552 collapsed two distinct binary-classification score-conversion paths into one:

• All-positive weights (RandomForest: leaf weights are probabilities ∈ [0,1]): label threshold > 0.5, complement via 1 - score → write_additional_scores ∈ {0, 1}.
• Mixed weights (gradient-boosted trees: weights can be negative): label threshold > 0, complement via -score → write_additional_scores ∈ {2, 3}.

After #27552, every binary case took the mixed-weights path. For RF where a leaf weight of 0.3 is a probability, the "complement" became -0.3, the label flipped at the wrong threshold, and downstream softmax/normalization either produced negative probabilities or row-sums ≠ 1. The LOGISTIC path is untouched here because sigmoid(score) / sigmoid(-score) produces a valid pair under either code path.

What's right

• The weights_are_all_positive_ flag is computed once during Init, threaded through the aggregator constructor, and consumed at score-write time. Correct layering.
• The flag computation walks target_class_ids (which it was iterating anyway) and folds in the positivity test. No extra pass over the weights.
• The tensor-vs-vector weight-source preference was made consistent with the leaf-building code in the same file after tianleiwu's review (target_class_weights_as_tensor.empty() ? static_cast<ThresholdType>(target_class_weights[i]) : target_class_weights_as_tensor[i]). That was the right call — diverging conventions between the positivity check and the actual leaf storage would have been a latent bug if anyone ever populated both arrays.
• The added static_cast<ThresholdType> matches the type used at the leaf-storage site, so the comparison happens at the same precision as the eventual score arithmetic.
• Member-init order in the constructor matches declaration order (tianleiwu confirmed). No -Wreorder warning.
• Three targeted tests cover all the meaningful cases:
  • score < 0.5 → label 0, complement [0.7, 0.3] (this is the case that previously produced [1-(-0.3), -0.3] = [1.3, -0.3]).
  • score > 0.5 → label 1, complement [0.2, 0.8].
  • score == 0.5 boundary → label 0 (since the test is strict > 0.5).

Comments

1. Boundary semantics — confirm with the spec. Test 3 pins score == 0.5 to label 0 (strict >). That matches the code (pos_weight > 0.5) and matches sklearn's behavior. Worth a comment in the test that this is deliberate and matches sklearn predict() ties-go-to-class-0 convention, in case a future reader thinks the boundary should round up.

2. The fix only addresses binary classification. The if (binary_case_) block is the only place using weights_are_all_positive_. Multi-class probabilities go through a separate path (write_scores / softmax) and weren't affected. The PR description should explicitly say "multi-class unaffected" so future bug triage doesn't suspect this code path again.

3. weights_are_all_positive_ is true for an empty weight vector (default-constructed bool starts as true, no iteration overwrites it). That's the right default — an empty model trivially has no negative weights — but a one-line comment "default = true; falsified by any negative weight" would help.

4. Test naming nit. TreeEnsembleClassifierBinaryAllPositiveWeightsNone reads as "no all-positive weights"; the trailing None refers to post_transform = NONE. Consider TreeEnsembleClassifierBinary_AllPositiveWeights_NonePostTransform for readability. Bikeshed.

5. The test rebuilds the full attribute set three times. A small lambda or helper would tidy this up, but for three tests it's fine as-is. Don't refactor.

6. The fix is narrow but the surface is wide. TreeEnsembleClassifier is consumed by every sklearn-to-ONNX conversion. Worth a follow-up to run the skl2onnx test suite against a build with this patch — that's the consumer most likely to have shape-of-the-bug variants that the three new tests didn't cover. Not blocking, but file an issue.

Things that look good

• The change is minimal and surgical: 1 added member, 1 added constructor arg, 1 added Init-time computation, 1 if (binary_case_) body restructured. ~30 lines of production code.
• The PR explicitly links both the broken-by issue (TreeEnsembleClassifier with post_transform=LOGISTIC skips sigmoid for leaf-only trees when all weights are non-negative #27533) and the broken-it PR (remove weights_are_all_positive_ from TreeEnsemble #27552), and notes that the LOGISTIC fix is preserved. Excellent provenance.
• Tests fail on main and pass with the patch (betatim asked for this explicitly and the author added them). That's the right gate.

Bottom line

Solid regression fix; correctly identifies the dropped invariant, restores it without re-breaking the original #27533 fix, and pins it down with tests that would catch a future regression. Approve. 85/85 CI green after the consistency commit.

[Tim Head (betatim)]

Thanks a lot!