Extend printer to handle non-empty domain#4372
Conversation
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
| if (node.domain() != "") | ||
| output_ << node.domain() << "."; |
There was a problem hiding this comment.
"ai.onnx" and "" both mean the default domain. Should ops with domain "ai.onnx" have consistent behavior with those with "" domain?
There was a problem hiding this comment.
yes, that's right. (Does it have any implication to this PR?)
There was a problem hiding this comment.
Does it have any implication to this PR?
Yes, maybe the line 323 can be changed to if (node.domain() != "" && node.domain() != "ai.onnx") if we want consistent behavior here
There was a problem hiding this comment.
I think it is better in the current form. Both forms are legal and mean the same thing. The parser/printer are just convenience utilities to convert the proto format to/from a readable textual form. It is useful to preserve the distinction between the two different, but equivalent, forms in the parser/printer.
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
4 participants
Signed-off-by: Ganesan Ramalingam grama@microsoft.com
Description
The printer does not currently handle a non-empty domain (used for ops in domains other than the standard one).
Extend the printer to handle it.