Skip to content

Regression in TreeEnsembleRegressor if the provided graph is a DAG #24636

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cbourjau opened this issue May 5, 2025 · 2 comments
Open

Regression in TreeEnsembleRegressor if the provided graph is a DAG #24636

cbourjau opened this issue May 5, 2025 · 2 comments

Comments

@cbourjau
Copy link
Contributor

cbourjau commented May 5, 2025

Describe the issue

Up until the introduction of the new TreeEnsemble operator, the ONNX standard did not support splitting on set memberships. This was problematic since such splits are commonly associated with categorical features. To support these split types, one was forced to compare each value in the set with an explicit BRANCH_EQ node. When applied naively, this would mean that the subtree behind the true-branch of each split had to be copied - a quite untenable situation in many scenarios.

The solution to this problem was to build a DAG rather than a Tree such that each true branch points to the same node (see ASCII diagram in the test case below). This used to work with onnxruntime<=1.20. However, it appears that a regression to these kinds of "trees" was introduced here. Thus, the following test case worked as intended on onnxruntime<=1.20 but fails on the latest 1.21.1 version.

To reproduce

import numpy as np
import onnxruntime as ort
import spox.opset.ai.onnx.ml.v3 as ml
from spox import Tensor, argument, build

# DAG structure:
#  __0
#  |  \
#  |___2
#  |    \
#  1     3
_NA = -1
kwargs = {
    "n_targets": 1,
    "nodes_truenodeids": [1, _NA, 1, _NA],
    "nodes_falsenodeids": [2, _NA, 3, _NA],
    "nodes_featureids": [0, _NA, 0, _NA],
    "nodes_missing_value_tracks_true": [1, _NA, 1, _NA],
    "nodes_modes": [
        "BRANCH_EQ",
        "LEAF",
        "BRANCH_EQ",
        "LEAF",
    ],
    "nodes_nodeids": [0, 1, 2, 3],
    "nodes_treeids": [0, 0, 0, 0],
    "nodes_values_as_tensor": np.array([0.0, _NA, 1.0, _NA], np.float64),
    "target_ids": [0, 1],
    "target_nodeids": [1, 3],
    "target_treeids": [0, 0],
    "target_weights_as_tensor": np.array([10.0, 20.0]),
}
inputs = np.array([[0.0], [1.0], [-1.0]])
expected = np.array([[10.0], [10.0], [20.0]], np.float32)

x = argument(Tensor(np.float64, ("N", 1)))
y = ml.tree_ensemble_regressor(x, **kwargs)

model = build({"x": x}, {"y": y})
with open("test.onnx", "wb") as f:
    f.write(model.SerializeToString())

session = ort.InferenceSession("test.onnx")
(result,) = session.run(None, {"x": inputs})

np.testing.assert_array_equal(result, expected)

Model file: test.onnx.zip

Urgency

High. This regression affects the execution of existing production models.

Platform

Mac

OS Version

14.7.5

ONNX Runtime Installation

Released Package

ONNX Runtime Version or Commit ID

1.21.1

ONNX Runtime API

Python

Architecture

ARM64

Execution Provider

Default CPU

Execution Provider Library Version

No response

@xadupre
Copy link
Member

xadupre commented May 5, 2025

Thanks for reporting it. I'll look into that tomorrow.

@xadupre
Copy link
Member

xadupre commented May 6, 2025

I found the issue and made a fix: #24654.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants