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

OP: enable Resize/Prelu/Sigmoid, modified Reshape #25

Merged
merged 11 commits into from Nov 13, 2020

Conversation

erizmr
Copy link
Contributor

@erizmr erizmr commented Nov 11, 2020

Hi @jackwish ,

The main contributions in this PR are listed below:

  1. Enable three new operators: Resize, Prelu, Sigmoid(seems that Sigmoid has been developed in another pull request, sorry for this redundant work..)
  2. Modified Reshape to adapt cases with one attribute
  3. Produce test models for three new operators (seems the model files are list in the .gitignore, so I excluded them from this PR )

Thanks for your awesome code review in advance :)

@zhenhuaw-me
Copy link
Owner

Hi @erizmr thanks for your contribution! It seems you forget to include the TFLite model file of resize_bilinear in this PR :)

@zhenhuaw-me zhenhuaw-me added the Operator Operator label Nov 11, 2020
@zhenhuaw-me zhenhuaw-me self-assigned this Nov 11, 2020
@zhenhuaw-me zhenhuaw-me added this to Proposed in v0.4 - Quantization Enhancement via automation Nov 11, 2020
@zhenhuaw-me zhenhuaw-me moved this from Proposed to In Progress in v0.4 - Quantization Enhancement Nov 11, 2020
@zhenhuaw-me
Copy link
Owner

For adding TFLite model files, you may need to use git add -f {file} as *.tflite is ignored by default.

@erizmr
Copy link
Contributor Author

erizmr commented Nov 11, 2020

Thanks! Have uploaded the missing models.

Copy link
Owner

@zhenhuaw-me zhenhuaw-me left a comment

Choose a reason for hiding this comment

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

@erizmr thank you for your great contribution! I have some minor comments, and I will read the Resize part tomorrow.

Btw, would you mind helping to update the Operator Support Status to demonstrate the new ops in this PR?

tflite2onnx/op/activation.py Show resolved Hide resolved
tflite2onnx/op/activation.py Outdated Show resolved Hide resolved
tflite2onnx/op/activation.py Outdated Show resolved Hide resolved
tflite2onnx/op/reshape.py Outdated Show resolved Hide resolved
tflite2onnx/op/reshape.py Outdated Show resolved Hide resolved
tflite2onnx/op/reshape.py Outdated Show resolved Hide resolved
@zhenhuaw-me zhenhuaw-me mentioned this pull request Nov 11, 2020
5 tasks
@codecov
Copy link

codecov bot commented Nov 11, 2020

Codecov Report

Merging #25 (c43b3bb) into master (268211b) will increase coverage by 0.18%.
The diff coverage is 95.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
+ Coverage   95.13%   95.31%   +0.18%     
==========================================
  Files          26       27       +1     
  Lines        1458     1559     +101     
==========================================
+ Hits         1387     1486      +99     
- Misses         71       73       +2     
Impacted Files Coverage Δ
tflite2onnx/op/reshape.py 91.66% <66.66%> (+0.17%) ⬆️
tflite2onnx/op/activation.py 97.00% <96.42%> (-0.27%) ⬇️
tflite2onnx/op/resize.py 98.55% <98.55%> (ø)
tflite2onnx/op/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 268211b...c43b3bb. Read the comment docs.

Copy link
Owner

@zhenhuaw-me zhenhuaw-me left a comment

Choose a reason for hiding this comment

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

@erizmr Thanks for your active work on this :) I have quick look at the Resize part and left several comments, but I need some time to check the interpolation in detail still.

The Codecov requires coverage don't drop > 1% (I am not sure if it's correctly configured though...), we may try to get that.

tflite2onnx/op/reshape.py Outdated Show resolved Hide resolved
tflite2onnx/op/resize.py Outdated Show resolved Hide resolved
tflite2onnx/op/resize.py Outdated Show resolved Hide resolved
tflite2onnx/op/resize.py Outdated Show resolved Hide resolved
tflite2onnx/op/resize.py Show resolved Hide resolved
tflite2onnx/op/resize.py Show resolved Hide resolved
tflite2onnx/op/resize.py Show resolved Hide resolved
tflite2onnx/op/resize.py Outdated Show resolved Hide resolved
tflite2onnx/op/resize.py Show resolved Hide resolved
tflite2onnx/op/resize.py Outdated Show resolved Hide resolved
@erizmr
Copy link
Contributor Author

erizmr commented Nov 13, 2020

Hi @jackwish , thanks for your review! Please check again.

Copy link
Owner

@zhenhuaw-me zhenhuaw-me left a comment

Choose a reason for hiding this comment

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

@erizmr LGTM in general, thanks for your contribution!

tflite2onnx/op/resize.py Show resolved Hide resolved
@zhenhuaw-me zhenhuaw-me merged commit 4157822 into zhenhuaw-me:master Nov 13, 2020
v0.4 - Quantization Enhancement automation moved this from In Progress to Done Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Operator Operator
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Potential bug: Reshape(one input, one attr) operator is not supported Enquiry about parsing optional input
3 participants