search

chainer / onnx-chainer

Add-on package for ONNX format support in Chainer
MIT License
85 stars 24 forks source link

added new tests #222

Closed durswd closed 4 years ago

disktnk commented 5 years ago

ONNXRuntime looks not to support broadcasting on PRelu, but PRelu test is split https://github.com/chainer/onnx-chainer/blob/v1.5.0/tests/functions_tests/test_activations.py#L56 so you can omit it, or add another test pattern on split it.

durswd commented 5 years ago

It seems that broadcasting test exists...

https://github.com/microsoft/onnxruntime/blob/24d17f43533c0e15ec267d52c6b2f995aa5de65b/onnxruntime/test/providers/cpu/activation/activation_op_test.cc#L173

disktnk commented 5 years ago

Looking the ONNXRuntime's test, shape of slope (= W in Chainer) is (num, 1), not (num,). If shape of W on this PR is (5, 1), not (5,), ONNXRuntime would be pass, but Chainer does not allow the dimension.

Actually add the below condition to convert_PReLUFunction, the test is passed.

def convert_PReLUFunction(
        func, opset_version, input_names, output_names, context):
    # ...
    elif opset_version == 7:
        w = func.inputs[1].get_variable()
        if broadcast_is_needed:
            slope = w[:, None]
            input_names[1] = context.add_param(slope, input_names[1])
        return onnx_helper.make_node('PRelu', input_names, output_names),
disktnk commented 4 years ago

To support Chainer's broadcasting on PRelu accepting different rank (=ndim), have to fix ONXN-Chainer converter. We need more discussion about this and out of scope of this PR, so close.

When we would get requirements about Chainer's broadcasting spec or increasing test coverage of chainer.functions, we'll support them on Chainer's repository. Thanks.