[Tracking] Spec clarifications

[jcwchen]

Creating this issue to track work items for spec clarifications. There are already multiple GitHub issues regarding op spec not being clear or error in expected outputs. I will collect them in this issue. Feel free to add more work item here. Thanks!

[jcwchen]

Old issues which look still valid:

Pooling related

• Inconsistent p range between LpNormalization, LpPool and GlobalLpPool: Inconsistent p range between LpNormalization, LpPool and GlobalLpPool #1614
• Ambiguity in Pool spec (AveragePool and MaxPool): Ambiguity in Pool spec (AveragePool and MaxPool) #1922
• Inconsistencies between MaxPool, AveragePool, and LpPool specs: Inconsistencies between MaxPool, AveragePool, and LpPool specs #2290
• MaxUnpool indices incompatible with PyTorch and inconsistent with MaxPool for kernels sizes which aren't a multiple of output size: MaxUnpool indices incompatible with PyTorch and inconsistent with MaxPool for kernels sizes which aren't a multiple of output size #2398
• operator MaxPool output tensor shape is ambiguously documented: operator MaxPool output tensor shape is ambiguously documented.  #2927
• Confused about AveragePool op ceil_mode attribute: Confused about AveragePool op ceil_mode attribute #3314

Conv/ConvTranspose related

• convtranspose with dynamic output shape: convtranspose with dynamic output shape #1772
• Order of kernel application in Conv & ConvTranspose: Order of kernel application in Conv & ConvTranspose #1886
• Padding specification unclear for Convolution/AveragePool/MaxPool: Padding specification unclear for Convolution/AveragePool/MaxPool #1651
• Output shape of Conv not specified: Output shape of Conv not specified #1795

RNN/LSTM related

• Documentation for RNN, LSTM, GRU shall specify output sequence order when direction is set to reverse: Documentation for RNN, LSTM, GRU shall specify output sequence order when direction is set to reverse #626
• GRU/RNN/LSTM's activation function: GRU/RNN/LGTM's activation function #1166
• Is RNN hidden_size attribute optional or not? Is RNN hidden_size attribute optional or not? #1213
• Spec bug to do with LSTM operator: Spec bug to do with LSTM operator #2116

Other operators

• [Document] The equivalent pseudo C code of loop: [Document] The equivalent pseudo C code of loop  #3389
• Rounding behavior of Resize when interpolating on integral types is unspecified: Rounding behavior of Resize when interpolating on integral types is unspecified #3390
• [ScatterElements] Clarify the requirement on indices uniqueness : [ScatterElements] Clarify the requirement on indices uniqueness #3234
• Please update SequenceToSplit spec with default value of 'split': Please update SequenceToSplit spec with default value of 'split'. #2396
• NonMaxSuppression spatial_dimension should be num_boxes: NonMaxSuppression spatial_dimension should be num_boxes? #2360
• The rank for "iou_threshold" and "score_threshold" of NonMaxSuppression is unclear: The rank for "iou_threshold" and "score_threshold" of NonMaxSuppression is unclear #2212
• Inconsistent ThresholdedRelu spec: Inconsistent ThresholdedRelu spec #1983
• Specification for TreeEnsembleClassifier and TreeEnsembleRegressor are inconsistent : Specification for TreeEnsembleClassifier and TreeEnsembleRegressor are inconsistent #1925
• Unsqueeze semantics not described precise enough: Unsqueeze semantics not described precise enough #1751
• Specifications for SVMClassifier is not precise enough : Specifications for SVMClassifier is not precise enough #1661
• Documentation on Gather operator unclear: Documentation on Gather operator unclear #1649
• Should ReduceL2/ReduceLogSum/ReduceLogSum only support float types? Should ReduceL2/ReduceLogSum/ReduceLogSum only support float types? #1458
• Scale attribute of Crop OP should be marked as optional: scale attribute of Crop OP should be marked as optional #1211
• Div operator behavoir for integers: Div operator behavoir for integers #1153

IR related

• Clarify meaning of operator Constant: Clarify meaning of operator Constant #1799
  Case sensitivity of operator names and attributes: Case sensitivity of operator names and attributes #1934
• Clarify treatment of Scalars vs 1D tesnors of size 1: Clarify treatment of Scalars vs 1D tesnors of size 1 #2428
• Clarification about TensorProto.segment: Clarification about TensorProto.segment #2630

[AlexandreEichenberger]

Conv operation does not specify if M (channel out) must be a multiple of number of groups. TF Keras and Pytorch have this restriction. #3641

[kraiskil]

Difference between Add and Sum could be more explicit, and that difference would be nice to have mentioned in both operator's documentation.
#339 says there was some differences with broadcasting support, but that doesn't seem to be the case anymore.

[tungld]

Expected outputs for Round looks incorrect #3755.

This one has been resolved. (Edited by @jcwchen)

[gramalingam]

The description of Cast operator could be improved (especially with conversions involving bool or string).

[jcwchen]

[Spec issue] Rounding or truncation behavior for Cast operator is not specified in the spec #3876

[gramalingam]

DequantizeLinear/QuantizeLinear: Spec should be more explicit if the inputs should be 2D or can be N-D, and explain the behavior in N-D case better.

[gramalingam]

QuantizeLinear: Spec should be more clear about the precision used for the different operators when using mixed-precision inputs (in computing x / y_scale + y_zero_point).

[volcacius]

LSTM: whenever input_forget=True, the spec doesn't specify which set of weights and biases ends up being used. It should be clarified that it's implementation dependent (e.g. ONNXRuntime uses the input ones and discards the forget ones https://github.com/microsoft/onnxruntime/blob/ef7b4dc05cae1084546ff7caba048a2f908ac1d8/onnxruntime/test/providers/cpu/rnn/LSTM.py#L208 ).

[garymm]

Prelu: The constraint rank(slope) == rank(X) OR slope is unidirectional broadcastable to X makes sense, but the spec doesn't explicitly say that. That is, it seems to allow rank(slope) > rank(X), which doesn't seem like it makes sense (if it does, could someone please explain?).

https://github.com/onnx/onnx/blob/main/docs/Operators.md#Prelu

[jcwchen]

I have a same question as #2303: "Do subgraph initializer names and input names shadow outer-scope names?" It's better to clarify it in the IR.

[gramalingam]

The specification of the "Pad" operator does not describe the intended behavior when there is an interaction between the use of negative pad-values (to truncate/slice) and the use of reflection (with a positive pad-value for the opposite side). Please see microsoft/onnxruntime#11828 for a detailed description.

[gramalingam]

The storage_order attribute of the MaxPool op seems out of place in the ONNX spec. Does the op need to be updated to eliminate/deprecate this attribute?

[AlexandreEichenberger]

Minor point, but naming of inputs matter in our onnx-mlir code base and we had to write extra templates because A & B for the MatMul, MatMulInteger, and QLinearMatMul are capitalized differently.

Use A & B:

• https://github.com/onnx/onnx/blob/main/docs/Operators.md#MatMul
• https://github.com/onnx/onnx/blob/main/docs/Operators.md#matmulinteger

Use a and b:

• https://github.com/onnx/onnx/blob/main/docs/Operators.md#QLinearMatMul

I know this may not matter to most, and it is not a big deal, but nevertheless name discrepancy between related ops for which, for example, shape inference can be "commoned" may face minor impediments due to naming conventions.

If this can be fixed without triggering incompatibilities between before/after renaming, then I am for it. If it would trigger incompatibilities, then let us just remember to use names as consistency as possible for future ops.

Thanks

[gramalingam]

If this can be fixed without triggering incompatibilities between before/after renaming, then I am for it. If it would trigger incompatibilities, then let us just remember to use names as consistency as possible for future ops.

My own guess would be that the names should not matter. But, paradoxically, it looks like it would matter for onnx-mlir.

I am in favor of using uniform naming conventions. If anyone has any concerns about changing parameter names of ops (without changing their version-number), please let us know.

[AlexandreEichenberger]

@gramalingam thanks for your answers

Found another case: Conv/ConvTranspose uses X() and W() but ConvInteger/QLinearConv uses x() and w()

[gramalingam]

Another couple of issues relating to Reduce* ops:

(a) I assume that the Reduce* ops should support the special-case of zero-rank tensors (with a single element). It would be good if the spec explicitly clarifies this.

(b) The spec does not indicate what happens if the input is a tensor with zero elements: e.g., a tensor of shape (100, 0) with reduction along the axis with size 0 (axis 1 in above example).
(i) The ideal answer for ops like Sum is 0, and Prod is 1. (The identity element for the op.)
(ii) However, for other ops like Max or Min, it can be complicated. An ideal answer for Max() is minus-infinity, but that is valid for only float-like types. For integral-types, it could be min-int.

In any case, some clarification whether this is allowed would be useful.

[justinchuby]

Max and Min can both take rank 0 tensors as inputs but it was not clear from the spec.

[edgchen1]

Regarding Pad op spec:

onnx/docs/Operators.md

Lines 17129 to 17142 in 0e9deba

	### <a name="Pad"></a><a name="pad">**Pad**</a>
	Given a tensor containing the data to be padded (`data`), a tensor containing the number of start and end pad values for axis (`pads`), (optionally) a `mode`, and (optionally) `constant_value`,
	a padded tensor (`output`) is generated.
	The three supported `modes` are (similar to corresponding modes supported by `numpy.pad`):
	1) `constant`(default) - pads with a given constant value as specified by `constant_value` (which defaults to 0, empty string, or False)
	2) `reflect` - pads with the reflection of the vector mirrored on the first and last values of the vector along each axis
	3) `edge` - pads with the edge values of array
	4) `wrap` - wrap-around padding as if the data tensor forms a torus

• should probably also mention optional axes input in the first sentence
• "The three supported modes are..." and then four are given

[edgchen1]

Regarding MeanVarianceNormalization op spec:

onnx/docs/Operators.md

Lines 15221 to 15226 in 0f53636

	#### Attributes
	<dl>
	<dt><tt>axes</tt> : list of ints (default is ['0', '2', '3'])</dt>
	<dd>A list of integers, along which to reduce. The default is to caculate along axes [0,2,3] for calculating mean and variance along each channel. Two variables with the same C-coordinate are associated with the same mean and variance.</dd>
	</dl>

• How should the default axes value be interpreted for inputs with fewer than four dimensions? The "C" reference seems specific to NCHW. This op does not otherwise seem limited to 4D input though.
• Are negative axis values supported?

[justinchuby]

• Pad operator behavior is not fully defined, examples lacking #4294

[justinchuby]

• What happens when axis of DFT - 17 is 0? #5493
• Document perm in Transpose need to have the same length as the input rank #5481