[Feature request] checking an input rank is within a specific range about onnx HOT 6 CLOSED

I think GridSample has been updated to support ND inputs (https://onnx.ai/onnx/operators/onnx__GridSample.html). To help me understand better, was your intention to support a range of input ranks during shape validation? What would this enable?

from onnx.

I think GridSample has been updated to support ND inputs (https://onnx.ai/onnx/operators/onnx__GridSample.html). To help me understand better, was your intention to support a range of input ranks during shape validation? What would this enable?

Yes your understanding is correct.

https://github.com/microsoft/onnxruntime/blob/3fb8905393bf91d97eae7afa0ae341828e3f1dbc/onnxruntime/core/graph/contrib_ops/contrib_defs.cc#L1044

If you look here, the logic is that it'll throw an error if you try to pass it input with anything but 4 dimensions. ORT is interested into extending this feature to support 5D, so (if my understanding is correct), this will need to change a bit to account for both 4D and 5D being valid inputs. If an operator is interested in supporting ND, would it not be helpful to have this validation?

Keep in mind this is a really small detail and I figured it was worth making a PR. I am still learning both ORT and ONNX - this isn't required functionality but I was mostly curious to see if it offered any utility. If I'm missing some context, please lmk and I can close both this PR and issue.

from onnx.

Thanks for the clarification. The new grid sample is going to support dimensions >5 as well, at least according to the spec. So specifying a range seems to be runtime specific. I would recommend adding this function only when it is needed for inferring shapes for onnx domain operations defined in this repository, to keep the APIs lean. Please correct me if there's anything I overlooked.

from onnx.

Thanks for the clarification. The new grid sample is going to support dimensions >5 as well, at least according to the spec. So specifying a range seems to be runtime specific. I would recommend adding this function only when it is needed for inferring shapes for onnx domain operations defined in this repository, to keep the APIs lean. Please correct me if there's anything I overlooked.

Sounds good, I don't think you've missed anything. Would you prefer this PR closed then?

(Tangential) can you point to some cxx related issues I can contribute towards if any exist? :)

from onnx.

Sounds good, I don't think you've missed anything. Would you prefer this PR closed then?

Yes. I think it's good to close it for now. Your contribution is appreciated still :)

(Tangential) can you point to some cxx related issues I can contribute towards if any exist? :)

Absolutely! Here is a list of issues that would welcome contributions: https://github.com/onnx/onnx/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc+label%3A%22contributions+welcome%22

Some c++ related ones are:

• #5222
• #5974
• #5843
• #5816

Etc.

from onnx.

Sounds good, I don't think you've missed anything. Would you prefer this PR closed then?

Yes. I think it's good to close it for now. Your contribution is appreciated still :)

(Tangential) can you point to some cxx related issues I can contribute towards if any exist? :)

Absolutely! Here is a list of issues that would welcome contributions: https://github.com/onnx/onnx/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc+label%3A%22contributions+welcome%22

Some c++ related ones are:

• Convert Split from opset11 to 18 failed. #5222
• onnx.checker.check_model() does not report mismatch input/output shapes #5974
• Create down conversion for GroupNormalization-21 #5843
• Inliner fails on the attached model #5816

Etc.

Much appreciated! I'll look into one of those :D

from onnx.