Bad Min conversion?

[vadimkantorov]

I'm trying to convert the openseq2seq's wav2letter model. It has tf.min(tf.relu(x), 20) activation function.
With default opset = 7 it looks like:

In the source it is explained that it does sth like: min(x -x + 20, x) in order to implement broadcasting. Note that value = 20 is present in the graph.

With opset = 10 it looks like:

Note that 20 is no longer present in node properties. Instead it refers to some ForwardPass/w2l_encoder/Minimum/y:0. I inspected it and it also does not contain the related attributes:

OP=Const
Name=ForwardPass/w2l_encoder/Minimum_17/y
Outpus:
        ForwardPass/w2l_encoder/Minimum_17/y:0=[], 10

name: "value"
t {
  data_type: 10
  name: "ForwardPass/w2l_encoder/Minimum_17/y:0"
  raw_data: "\000M"
}
type: TENSOR

[zhijxu-MS]

@vadimkantorov wav2letter seems to be a public model, would you mind share with me the link to download it?

[lucienwang1009]

@vadimkantorov Min of opset >= 8 supports broadcasting so we don't need to do something like min(x -x + 20, x). And ForwardPass/w2l_encoder/Minimum/y does contain the value 20 in its raw_data.
Does this conversion in your model cause some errors?

[vadimkantorov]

@zhijxu-MS here is the link: https://drive.google.com/file/d/10EYe040qVW6cfygSZz6HwGQDylahQNSa/view?usp=sharing

[vadimkantorov]

Here is the onnx file that I got using my conversion script (using tf2onnx under the hood):
w2l_plus_large_mp.zip

[lucienwang1009]

@vadimkantorov Thanks for the detailed info.
As I said above, it should be two different conversions of Minimum on different onnx opsets.
If there is no running error or mismatched result, the conversion should be correct.

[vadimkantorov]

@lucienwang1009 I understand the different ways of conversion for opset7 and opset10. I was just worried that value = 20 was not visible in Netron visualizer (when converted for opset10). But if you say it's present in the onnx graph, then please feel free to close this issue.

Anyway currently I'm blocked with tf2onnx usage by: #572 and #571 (TensorFlow uses BatchToSpaceND for representing dilated convolutions).

Also Min(ReLU(x), const) could be rewritten as onnx.Clip(0, const) op, but it's a minor issue.

Thanks!

[lucienwang1009]

@vadimkantorov Thanks for a deal of feedback :) We will deal with them soon. For optimization enhancement, please feel free to file an issue.