search

kraiskil / onnx2c

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

Add local matmul precision test #2

Closed robinvanemden closed 3 years ago

robinvanemden commented 3 years ago

The local matmul test in the current pull request fails (for me) when not also adding std::cout.precision(20) to onnx_backend_tests_runner.cc (I also added it to main.cc).

The data is generated by a basic Python script that I use to create simple models together with their input and onnxruntime generated output.

It is probably a good idea to double check whether a precision of 20 offers the right balance between c file size and model precision.

kraiskil commented 3 years ago

This does indeed show the failure due to precision - the tests fail. Now we would also need the fix so that the tests pass again ;)

Also, I realized since there is more than one contributor now, onnx2c needs a license, so I added one. If the terms are acceptable to you, please rebase this PR on master and add yourself to the blamelist.

robinvanemden commented 3 years ago

The terms seem fine to me! I rebased the PR on master, and added the decimal precision fix as well to make sure the test now passes.

kraiskil commented 3 years ago

Merged, thanks! :+1:

I did a bit of sin, and rebased your commits on master to get rid of the merge commit message. Also, I switched the order so that the fix commit comes first. This way if bisecting is needed, the test suite won't fail because of this known issue you fixed. Since github tells me your fork is the only one, this history rewriting shouldn't be that bad a thing. Sorry for the trouble!

robinvanemden commented 3 years ago

No problem at all - your reordering of the commit makes sense 😄 Thanks for merging!