Fix TreeEnsembleRegressor to add base_values to final prediction. Round 2. #5569
Conversation
…ate docs to clarify convention. Signed-off-by: Corwin Joy <corwinjoy@gmail.com>
|
Redo of the previous PR to cleanup commit history. |
|
There are some CI failures. Looks like an erroneous opset version (4) problem? Maybe we need to specify the target opset version for the newly added tests (in the reference implementation)? |
|
I did not have time to investigate. If we specify opset 4 for ai.onnx.ml it should pick the operator defined in opset 3. It looks like a bug. |
|
This issue seems more complex. The tests are passing with the external protobuf but not with the internal version. |
|
I proposed a fix in #5665, the only change is https://github.com/onnx/onnx/pull/5665/files#diff-d0acfbc52fc9a25dab6fb8f2057b06d2ccf075f37129c1fac460ca8564c55597R833: @parameterized decorator must be placed before @UnitTest one. #5665 can be checked in or merged into this one. |
|
@xadupre Thanks for investigating! I couldn't tell why the tests were failing. I am happy to go with your updated PR! |
Pull request was closed
This PR fixes one line failing on #5569. What follows comes from this PR description. ### Description In the reference implementation of TreeEnsembleRegressor when a value is provided for the base_values argument, this value replaces any prediction. This can't be what's intended and does not match onnxruntime. Instead, the base_value should be added to the prediction after applying any aggregation. ### Motivation and Context We are exporting regression trees into ONNX that have a non-zero base_value as the baseline prediction for the tree. Prediction works as expected in onnxruntime but not in the reference implementation. I believe this is an oversight and propose the fix (plus tests) below. I also think the documentation should be more explicit about what the base_values argument does. --------- Signed-off-by: Corwin Joy <corwinjoy@gmail.com> Signed-off-by: Xavier Dupre <xadupre@microsoft.com> Co-authored-by: Corwin Joy <corwin.joy@gmail.com> Co-authored-by: G. Ramalingam <grama@microsoft.com>
Description
In the reference implementation of TreeEnsembleRegressor when a value is provided for the
base_valuesargument, this value replaces any prediction. This can't be what's intended and does not match onnxruntime. Instead, thebase_valueshould be added to the prediction after applying any aggregation.Motivation and Context
We are exporting regression trees into ONNX that have a non-zero
base_valueas the baseline prediction for the tree. Prediction works as expected in onnxruntime but not in the reference implementation. I believe this is an oversight and propose the fix (plus tests) below. I also think the documentation should be more explicit about what thebase_valuesargument does.