-
Notifications
You must be signed in to change notification settings - Fork 397
Update SuperRes ios app to released version ort-ext pods and some other minor refinements #208
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
Conversation
|
I also tried to update the way to register custom ops using function name approach here , but got a symbol not found error, not sure if any extra set up is required: Lines 32 to 34 in 34de606
using: |
| # Pre-release version pods | ||
| pod 'onnxruntime-extensions-c', '0.5.0-dev+261962.e3663fb' | ||
| # Release version pod for ort-ext | ||
| pod 'onnxruntime-extensions-c', '0.6.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just use the latest?
| pod 'onnxruntime-extensions-c', '0.6.0' | |
| pod 'onnxruntime-extensions-c' |
natke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor comments
|
|
||
| - [Android Super Resolution](examples/super_resolution/android) | ||
| - [iOS Super Resolution](examples/speech_recognition/ios) | ||
| - [iOS Super Resolution](examples/super_resolution/ios) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot!
| objects = { | ||
|
|
||
| /* Begin PBXBuildFile section */ | ||
| 510A53D6296DE390000DB268 /* pt_super_resolution_with_pre_post_processing_opset16.onnx in Resources */ = {isa = PBXBuildFile; fileRef = 510A53D5296DE390000DB268 /* pt_super_resolution_with_pre_post_processing_opset16.onnx */; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this file be under source control?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the older opset16 version model, do we still want to keep that model in the project?
also fyi .pbxproj file updates are auto-generated after updating the ios app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer we only have the opset 18 model given that will have significantly better output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also fyi .pbxproj file updates are auto-generated after updating the ios app.
That's why I wondered the pbxporj file should be under version control? If it gets re-generated. Not a biggie
| 5130D9C32968B93C009B4B88 /* Preview Assets.xcassets in Resources */, | ||
| 5130D9C02968B93C009B4B88 /* Assets.xcassets in Resources */, | ||
| 510A53E8296FE46F000DB268 /* cat_224x224.png in Resources */, | ||
| 5197BE9E29A026B3004B83DE /* pytorch_superresolution_with_pre_post_processing_op18.onnx in Resources */, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 'op18' is a bit obtuse vs 'opset18'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True! I did confuse op16 with fp16 :-)
| } | ||
|
|
||
| if performSuperRes { | ||
| Text("Output high resolution image: ").frame(width: 350, height: 40, alignment:.leading) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: would 'higher resolution image' lower user expectations of extra special magic happening?
| objects = { | ||
|
|
||
| /* Begin PBXBuildFile section */ | ||
| 510A53D6296DE390000DB268 /* pt_super_resolution_with_pre_post_processing_opset16.onnx in Resources */ = {isa = PBXBuildFile; fileRef = 510A53D5296DE390000DB268 /* pt_super_resolution_with_pre_post_processing_opset16.onnx */; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer we only have the opset 18 model given that will have significantly better output.
| 5130D9C02968B93C009B4B88 /* Assets.xcassets in Resources */ = {isa = PBXBuildFile; fileRef = 5130D9BF2968B93C009B4B88 /* Assets.xcassets */; }; | ||
| 5130D9C32968B93C009B4B88 /* Preview Assets.xcassets in Resources */ = {isa = PBXBuildFile; fileRef = 5130D9C22968B93C009B4B88 /* Preview Assets.xcassets */; }; | ||
| 5197BEA129A0492D004B83DE /* pytorch_superresolution_with_pre_post_processing_opset18.onnx in Resources */ = {isa = PBXBuildFile; fileRef = 5197BEA029A0492D004B83DE /* pytorch_superresolution_with_pre_post_processing_opset18.onnx */; }; | ||
| 5197BEA229A0492D004B83DE /* pytorch_superresolution_with_pre_post_processing_opset18.onnx in Resources */ = {isa = PBXBuildFile; fileRef = 5197BEA029A0492D004B83DE /* pytorch_superresolution_with_pre_post_processing_opset18.onnx */; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are there two entries here for pytorch_superresolution_with_pre_post_processing_opset18.onnx? was it added twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are there two entries here for pytorch_superresolution_with_pre_post_processing_opset18.onnx? was it added twice?
I think one is for ORTSuperResolution and one is for ORTSuperResolutionTest. I added the model to both targets.
shahicasper
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apply
Uh oh!
There was an error while loading. Please reload this page.