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

VALID auto_pad value #382

Open
igor-yusupov opened this issue Oct 14, 2024 · 8 comments
Open

VALID auto_pad value #382

igor-yusupov opened this issue Oct 14, 2024 · 8 comments

Comments

@igor-yusupov
Copy link
Contributor

I found an interesting model for removing objects from image. I'm going to add it to comparisons-rten repo, I already prepared python code. But I got an error with converting:

Error converting "Constant" operator: Unsupported tensor data type uint8 for operator /Constant_8_output_0

weights link

@igor-yusupov
Copy link
Contributor Author

Btw as I see this operator just returning constant value? https://github.com/onnx/onnx/blob/2b504f81f1cadb098e69b53b76e4e12b66a3bdbf/onnx/reference/ops/op_constant.py#L57

I can try to add it if you think that it's easy

@robertknight
Copy link
Owner

Constant operators in ONNX models are eliminated when converting to .rten and replaced with the value (weights, bias etc.) they emit. They end up as ConstantNode nodes in the converted graph. See the constant_node_from_onnx_constant_op function. In the main branch converting this uint8 Constant succeeds because int8 and uint8 tensors are now supported.

However there is a different error that gets reported after:

Error converting Conv operator /gaussian_blur/Conv: Unsupported auto_pad value VALID
Traceback (most recent call last):
  File "/Users/robert/.virtualenvs/wasnn/bin/rten-convert", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/Users/robert/projects/rten/rten-convert/rten_convert/converter.py", line 1557, in main
    graph = graph_from_onnx_graph(model.graph)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/robert/projects/rten/rten-convert/rten_convert/converter.py", line 1148, in graph_from_onnx_graph
    raise ValueError(
ValueError: Errors occurred when converting 1 operators

From the list of values given in the spec, only the "NOTSET" and "SAME_UPPER" values are currently supported.

The latest version of the Conv operator mentions "VALID" but doesn't say what it means. Older versions on the same page say VALID means no padding. Internally that could be handled similar to the way Padding::Fixed works when all the values are zero. The difference is that the number of padding sides isn't specified in the model (it adapts to the number of spatial dimensions in the input tensor). Feel free to have a go at implementing that.

@igor-yusupov igor-yusupov changed the title Support Constant operator VALID auto_pad value Oct 14, 2024
@igor-yusupov
Copy link
Contributor Author

Thank you for response, I'll try it

@igor-yusupov
Copy link
Contributor Author

Found an illustration of how VALID pading works. It seems that with stride=1 this mode is no different from just no padding. So I just removed that parameter and re-converted the onnx weights.

After that I ran into a bug during inference, some operators didn't handle int8, so I made a PR with fixes for that: #387

@robertknight
Copy link
Owner

So I just removed that parameter and re-converted the onnx weights.

Do you have a link to the re-converted weights?

After that I ran into a bug during inference, some operators didn't handle int8, so I made a PR with fixes for that: #387

Thanks. Does the model work as intended with those changes applied?

@igor-yusupov
Copy link
Contributor Author

Yes, it works ok with the example images. I added migan example in comparisons-rten. I have also provided links to images and weights

@igor-yusupov
Copy link
Contributor Author

I looked at the weights with the netron app and saw that in this padding stride=1, so it seems it doesn't make sense to specify padding='valid'. So I removed the parameter in this line of code

Moreover on my machine with this parameter I get an error when trying to convert weights. Maybe this parameter is for older versions of packages?

@igor-yusupov
Copy link
Contributor Author

btw the author refers to this code and it doesn't have this parameter

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

No branches or pull requests

2 participants