search

kraiskil / onnx2c

Open Neural Network Exchange to C compiler.
Other
184 stars 30 forks source link

Add Concat op. #4

Closed robinvanemden closed 3 years ago

robinvanemden commented 3 years ago

All ONNX backend tests pass. First op that contains variadic inputs.

kraiskil commented 3 years ago

Thanks for the PR.

I noticed that in the generated test case the concat node lacks the output tensor given as a parameter. The tests still pass, since there is a global reserved tensor of the same name (it is intended that a pointer to this is passed in as a parameter).

There are some pytorch-generated tests in the backend test suite too, one of which tests concat - and this one fails: ONNX_type_test(operator_concat2 ${ONNX_BACKEND_TEST_DATA_DIR}/pytorch-operator/test_operator_concat2 ONNX_backend_pytorch_conv 0.00001 0)

kraiskil commented 3 years ago

Also, the generated code lacks some indentations, which make it a bit difficult to read. Tabs are preferred, since that allows the reader to adjust the indentation depth to their preference, and keeps the indentation consistent across nodes.

robinvanemden commented 3 years ago

Thanks for the feedback. I will make the suggested changes asap.

robinvanemden commented 3 years ago

I made the requested changes and cleaned up the code some more. Thanks again for the review!