search

kraiskil / onnx2c

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

Fixes stride calculation in MaxPool for indices output #21

Closed kernhanda closed 2 years ago

kernhanda commented 2 years ago

The current implementation breaks for the attached ONNX model

MaxPool.zip

robinvanemden commented 2 years ago

I created a small test for this PR (see attached). It confirms the PR fixes the stride calculation for the indices output.

maxpool_stride_calulation_test.zip .

kraiskil commented 2 years ago

Well spotted! Looking at the maxpool.h file now, and its pretty much gibberish now to me. I should have commented it better :|

@robinvanemden thanks for the test. I added it locally, and without @kernhanda 's patch, it indeed failed. With the patch it passed. So I merged this PR in.

However, now after merging this in, I'm re-trying the test, and it fails: in the generated test suite, both tensor_output_0 and tensor_reference_0 are of shape [3][223][223], where as the entry function expects output Y to be of shape [1][3][223][223].

I'm not quite sure what is going on here, I'll have to have a closer look. It might be related to the recent commit a3a6b2a3176cad4f06a9115aa6144febb0826f9e that changes how (missing) batch sizes (i.e. the dimension are treated. For now, I added the contents of the zip to branch pr21 (8224d134e06fc812804db568beaa7aef6494b3e1).

kraiskil commented 2 years ago

I added regression tests for this fix in 90a97945e0aaa60da1dd5b98257a4161f0636379

robinvanemden commented 2 years ago

Super, thank you!