Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hasInput method to InferenceContext #4451

Merged
merged 5 commits into from Aug 24, 2022
Merged

Conversation

gramalingam
Copy link
Contributor

See #4447 for a discussion.

Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam gramalingam requested a review from a team as a code owner August 17, 2022 21:29
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
Copy link
Contributor

@p-wysocki p-wysocki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I think it's a great addition.

@gramalingam
Copy link
Contributor Author

@daquexian can you please take a look? This requires the approval from someone in operator SIG. Thanks!

Copy link
Member

@jcwchen jcwchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great improvement! Shall we also apply hasInput in

  1. https://github.com/onnx/onnx/blob/main/onnx/defs/tensor/utils.cc#L95-L99
  2. https://github.com/onnx/onnx/blob/main/onnx/defs/quantization/old.cc#L41

Besides, do you think it is also useful if we can have another common function for ctx.getNumInputs() > x && hasInputShape(ctx, x)? For example: https://github.com/onnx/onnx/blob/main/onnx/defs/nn/old.cc#L70-L77 It can be done by another PR if needed.

Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam
Copy link
Contributor Author

Great improvement! Shall we also apply hasInput in

  1. https://github.com/onnx/onnx/blob/main/onnx/defs/tensor/utils.cc#L95-L99
  2. https://github.com/onnx/onnx/blob/main/onnx/defs/quantization/old.cc#L41

Besides, do you think it is also useful if we can have another common function for ctx.getNumInputs() > x && hasInputShape(ctx, x)? For example: https://github.com/onnx/onnx/blob/main/onnx/defs/nn/old.cc#L70-L77 It can be done by another PR if needed.

Added the first two, thanks! As for the next suggestion, hasInputShape already works this way, so we don't need a new function I think. (The extra condition ctx.getNumInputs() > x is not needed.)

Copy link
Member

@linkerzhang linkerzhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good improvement!

Copy link
Member

@daquexian daquexian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

@jcwchen jcwchen merged commit 9b8c8d2 into onnx:main Aug 24, 2022
@gramalingam gramalingam deleted the has-input branch August 24, 2022 05:24
broune pushed a commit to broune/onnx that referenced this pull request May 6, 2023
* Add hasInput method to InferenceContext

Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>

* Fix const

Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>

* Replace other uses

Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>

Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
Co-authored-by: Ke Zhang <linkerzhang@yeah.net>
Co-authored-by: Chun-Wei Chen <jacky82226@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants