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

Make torch_stft_fb.py encoder onnxable #11

Merged
merged 1 commit into from
Mar 12, 2021
Merged

Conversation

mpariente
Copy link
Contributor

Ref #10
The fix was quite simple, but it always takes me time to find scriptable and onnxable tricks.

@mpariente
Copy link
Contributor Author

And I cannot make the Decoder onnxable though.
If you want to give it a try @faroit, feel free!

@faroit
Copy link
Contributor

faroit commented Feb 21, 2021

Ref #10
The fix was quite simple, but it always takes me time to find scriptable and onnxable tricks.

cool, how did you find out that this causes problems?

@faroit
Copy link
Contributor

faroit commented Feb 21, 2021

@mpariente shall we add separate onnx tests for encoder and decoder, thus making at least something pass for this PR? I can then work on the decoder

@mpariente
Copy link
Contributor Author

cool, how did you find out that this causes problems?

By elimination.

@mpariente shall we add separate onnx tests for encoder and decoder, thus making at least something pass for this PR? I can then work on the decoder

Yes, it would be nice to add tests.
I think they only work with nightly though, I'd have to check.
Do you want to implement them?

@faroit
Copy link
Contributor

faroit commented Mar 12, 2021

@mpariente good news, with your fix + torch 1.8.0 + setting opset to 11, both the decoder and encoder passes the onnx export.
We could go lower than opset 11 (which would allow better conversion to tensorflow) when the .squeeze(0) would be replaced...

UserWarning: This model contains a squeeze operation on dimension 0. If the model is intend
ed to be used with dynamic input shapes, please use opset version 11 to export the model.

I any case, i think separate tests should be added in a separate PR, i can do that. So I would suggest to merge this first

@mpariente mpariente merged commit 3510292 into master Mar 12, 2021
@mpariente
Copy link
Contributor Author

I'd love tests, thanks !

@mpariente mpariente deleted the torch_stft_onnxable branch April 9, 2021 18:54
@mpariente mpariente restored the torch_stft_onnxable branch April 9, 2021 18:54
@mpariente mpariente deleted the torch_stft_onnxable branch April 9, 2021 18:54
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