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

Fix/gemm rewriter bias add #1069

Merged

Conversation

phager90
Copy link
Contributor

Fixing #1068
Test will pass always due to an unfixed FIXME in check_op_count(..)

Copy link
Contributor

@TomWildenhain-Microsoft TomWildenhain-Microsoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the rewrite_gemm should be moved to the end of the rewriters list (tfonnx.py line 454). Currently it is messing up a pattern that the LSTM rewriter looks for.

tests/test_backend.py Outdated Show resolved Hide resolved
phager90 and others added 2 commits August 21, 2020 11:16
Co-authored-by: TomWildenhain-Microsoft <67606533+TomWildenhain-Microsoft@users.noreply.github.com>
@phager90
Copy link
Contributor Author

I moved the rewriter back as you suggested. I will check later the status of the unit tests.

BTW: what about the unit test that return true always? (see #1068)

@TomWildenhain-Microsoft TomWildenhain-Microsoft merged commit 1a35937 into onnx:master Aug 21, 2020
@TomWildenhain-Microsoft
Copy link
Contributor

Great! The graph validator should be fixed at some point but isn't actually that important. The unit testing system runs the converted graph through ONNX Runtime and compares the result with running the original through TF. The validator is just an additional check to make sure certain optimizations get made. If the unit test passes, we know conversion succeeds and gives the correct result.

@phager90
Copy link
Contributor Author

Thanks for the clarifications!

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.

None yet

2 participants