kraiskil/onnx2c

Operators with variadic inputs (or outputs)

robinvanemden opened this issue · 6 comments

robinvanemden commented

Since onnx2 already supports optional inputs and outputs (10e92e9 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

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

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

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

kraiskil commented

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

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

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.