fix: ONNX conversion for Spark OneHotEncoder model #552
Conversation
|
This pull request introduces 1 alert and fixes 1 when merging 0112e2d into f87e306 - view on LGTM.com new alerts:
fixed alerts:
|
Signed-off-by: Jason Wang <jasowang@microsoft.com>
|
This pull request fixes 1 alert when merging 446b7a9 into f87e306 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging bcabd14 into f87e306 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging f40b4ec into f87e306 - view on LGTM.com fixed alerts:
|
Signed-off-by: Jason Wang <jasowang@microsoft.com>
Signed-off-by: Jason Wang <jasowang@microsoft.com>
… converter Signed-off-by: Jason Wang <jasowang@microsoft.com>
|
This pull request fixes 1 alert when merging e297f6e into f87e306 - view on LGTM.com fixed alerts:
|
| @@ -19,4 +19,4 @@ pytest-spark | |||
| scikit-learn | |||
| scipy | |||
| wheel | |||
| xgboost | |||
| xgboost==1.5.2 | |||
There was a problem hiding this comment.
The latest xgboost version breaks some unit tests in the xgboost unit tests, which has nothing to do with my changes, so I'm pinning it to the older version that works.
| shape_variable_name = scope.get_unique_variable_name("shape_at_" + str(i)) | ||
| container.add_initializer(shape_variable_name, onnx_proto.TensorProto.INT64, [2], [N, len(categories[i])]) | ||
|
|
||
| container.add_node( |
There was a problem hiding this comment.
Why switch to an output per category and not concatenate them into one single output?
There was a problem hiding this comment.
This is to match the behavior of the Spark OneHotEncoderModel. In Spark, if the OHEModel has multiple outputCols (one per category), the model does not concat the outputs into a vector. Instead you use a vector assembler to concat them, and the vector assembler will be translated into a concat ONNX operator.
Signed-off-by: Jason Wang <jasowang@microsoft.com>
|
This pull request fixes 1 alert when merging c27c453 into f87e306 - view on LGTM.com fixed alerts:
|
|
@xadupre can you merge the change? |
Fixing issue #551.