Skip to content

Liqun/optional input#4326

Merged
liqunfu merged 6 commits intoonnx:function-experimentfrom
liqunfu:liqun/optional_input
Jul 21, 2022
Merged

Liqun/optional input#4326
liqunfu merged 6 commits intoonnx:function-experimentfrom
liqunfu:liqun/optional_input

Conversation

@liqunfu
Copy link
Copy Markdown
Contributor

@liqunfu liqunfu commented Jun 29, 2022

Description

  • Describe your changes.

Motivation and Context

  • Why is this change required? What problem does it solve?
  • If it fixes an open issue, please link to the issue here.

liqunfu added 3 commits June 21, 2022 15:17
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
@liqunfu liqunfu requested review from a team as code owners June 29, 2022 19:13
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Comment thread onnx/__init__.py
'''
'''
if isinstance(proto, bytes):
proto = _deserialize(proto, LibProto())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why deserialize and serialize back?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. TODO add so that I can make the codel change when working with LibProto - I need to merge this branch in order to build ort with my optional input branch.

Comment thread onnx/onnx.in.proto Outdated

optional string domain = 4;

optional int64 model_version = 5;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be lib_version ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Adding comments just like in ModelProto would be helpful.

Comment thread onnx/defs/schema.h
"optional(tensor(float))", "optional(tensor(double))", "optional(tensor(string))",
"optional(tensor(bool))", "optional(tensor(complex64))", "optional(tensor(complex128))"};
"optional(tensor(bool))", "optional(tensor(complex64))", "optional(tensor(complex128))",
"tensor(float)"};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't understand this. Is it here by mistake?

@liqunfu liqunfu merged commit 4f8a1b6 into onnx:function-experiment Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants