search

kraiskil / onnx2c

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

Operators with variadic inputs (or outputs) #3

Closed robinvanemden closed 3 years ago

robinvanemden commented 3 years ago

Since onnx2 already supports optional inputs and outputs (https://github.com/kraiskil/onnx2c/commit/10e92e9fad2b3a3e61ce615fd62578280cd203b7 is nice), operators with variadic inputs (or outputs) seem within reach. What do you think would be an elegant way to implement these operators (such asmin, max and mean)?

kraiskil commented 3 years ago

Ugh. I didn't realize variadic inputs was a possibility too. I had a quick glance over the ONNX documentation, and I don't see a big problem here. Do I understand correctly that the number of input tensors is variable only in the specification, but always fixed in a given graph?

If the above is true, then the implementation of those nodes could keep track of their inputs as a std::vector<Tensor *>. I don't think there would even be any need for changes elsewhere in the sources.

robinvanemden commented 3 years ago

Thanks! You are correct: the number of input tensors is always fixed in a given graph. And I am indeed able to keep track of inputs using a std::vector<Tensor *> in a first, rough version of the concat operator (which completes one of its 2d tests now). I will create a pull request when I've finished it. As an aside: maybe it would be an option to rename the resolveOutput() function, as it assigns inputs as well?

robinvanemden commented 3 years ago

I just completed the concat op! It now passes all ONNX backend tests - see the pull request here.

kraiskil commented 3 years ago

As an aside: maybe it would be an option to rename the resolveOutput() function, as it assigns inputs as well?

Yes, the name could be a bit misleading. I think that initially resolveOutput() just created the output tensors, but the functionality grew a bit over time. Is just plain 'resove()' more descriptive? Or suggestions welcome - I fear my point of view has become a bit entrenched :)

robinvanemden commented 3 years ago

As an aside: maybe it would be an option to rename the resolveOutput() function, as it assigns inputs as well?

Yes, the name could be a bit misleading. I think that initially resolveOutput() just created the output tensors, but the functionality grew a bit over time. Is just plain 'resove()' more descriptive? Or suggestions welcome - I fear my point of view has become a bit entrenched :)

I think resolve() would be perfect! It's really only the "output" part that briefly threw me off :)

kraiskil commented 3 years ago

Merged the PR, thanks. Again, a squash merge, sorry for the history re-write. I seem to have a soft spot for clean git history lines...

I made a separate issue for the renaming, so this issue can be closed.