-
-
Notifications
You must be signed in to change notification settings - Fork 16.5k
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
Add NMS to CoreML model output, works with Vision #7263
Conversation
Reference issues: #5157 , #343 , #7011 The current version of the export.py script outputs a CoreML model without NMS. This means that certain Vision APIs cannot be used with the model directly, as the output during inference is VNCoreMLFeatureValueObservation. The changes implemented here add a NMS layer to the CoreML output, so the output from inference is VNRecognizedObjectObservation. By adding NMS to the model directly, as opposed to later in code, the performance of the overall image/video processing is improved. This also allows use of the "Preview" tab in Xcode for quickly testing the model. Default IoU and confidence thresholds are taken from the `--iou-thres` and `--conf-thres` arguments during export.py script runtime. The user can also change these later by using a CoreML MLFeatureProvider in their application (see [https://developer.apple.com/documentation/coreml/mlfeatureprovider](https://developer.apple.com/documentation/coreml/mlfeatureprovider)). This has no effect on training, as it only adds an additional layer during CoreML export for NMS.
for more information, see https://pre-commit.ci
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.
👋 Hello @mshamash, thank you for submitting a YOLOv5 🚀 PR! To allow your work to be integrated as seamlessly as possible, we advise you to:
- ✅ Verify your PR is up-to-date with upstream/master. If your PR is behind upstream/master an automatic GitHub Actions merge may be attempted by writing /rebase in a new comment, or by running the following code, replacing 'feature' with the name of your local branch:
git remote add upstream https://github.com/ultralytics/yolov5.git
git fetch upstream
# git checkout feature # <--- replace 'feature' with local branch name
git merge upstream/master
git push -u origin -f
- ✅ Verify all Continuous Integration (CI) checks are passing.
- ✅ Reduce changes to the absolute minimum required for your bug fix or feature addition. "It is not daily increase but daily decrease, hack away the unessential. The closer to the source, the less wastage there is." -Bruce Lee
I tried your new method, which was very effective. During the exporting, I noticed some warning,But it has no effect on the results TorchScript: starting export with torch 1.9.1... |
@liuzhiguai - Thanks for reporting back! I don't think I had those errors during export, but I have made a couple modifications since, and integrated with the current version of the export.py script as well. Can you please try again with the newest version of the export script, which I am submitting in this PR? You can download the file here: https://github.com/mshamash/yolov5/blob/fix/coreml_export_nms_layer/export.py |
I tried the newest version,There was no warning this time.This is very helpful to me Traceback (most recent call last): |
@liuzhiguai - I didn't change anything on that line, so I'm not sure why it's erroring. Perhaps do a |
@glenn-jocher is this something that you think could be implemented/merged in the export.py script? |
@mshamash yes, but we have a higher level issue. Right now all model exports are benchmarkable, i.e. see #6613: MacOS Intel CPU Results (CoreML-capable)
There have been user requests for native-NMS exports in a few formats, i.e. TFLite, ONNX, TRT, TorchScript, and here with CoreML. So we need additional infrastructure within val.py, detect.py, PyTorch Hub, and/or the NMS function to recognize native-NMS output formats and handle these accordingly to allow these to also work correctly with the various inference pathways. |
Thank you! |
Thank you, works fine on my side :) |
+1 |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions YOLOv5 🚀 and Vision AI ⭐. |
not stale |
nms.iouThreshold = iou_thres | ||
nms.confidenceThreshold = conf_thres | ||
nms.pickTop.perClass = False | ||
nms.stringClassLabels.vector.extend(labels) |
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.
mshamash if you end up merging newer commits from the main branch to this branch, these labels
could/should be changed to labels.values()
since it's a dictionary now 👍
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.
@hietalajulius don't forget that a dictionary is not ordered. I guess it's better to do [labels[k] for k in sorted(labels.keys())]
Any possibility of implementing this (NMS layer for coreml)? @glenn-jocher |
@hietalajulius Can you have a look at this one please? |
user_defined_metadata = { | ||
"iou_threshold": str(iou_thres), | ||
"confidence_threshold": str(conf_thres), | ||
"classes": ", ".join(labels)} |
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.
@hietalajulius @mshamash Also relevant here.
@mshamash Do you know why the confidence shown in Xcode is always 100%? Isn't there a way to output the confidence returned by the NMS layer for the BBox? |
Any reason this wonderfully working fix wasn't merged yet? |
Agree with @Jaykob because the mlmodel that comes out of |
@philipperemy did you figure out why you were getting 100% confidence? I ran into the same issue.. confidence always 98-100% |
@wmcnally nope. I gave it on that one. Mine was always 100%. |
@wmcnally @philipperemy Are you guys testing on the same images you trained with? Confidence worked just fine for me, however I had to manually apply @mshamash 's changes to master coz I made a couple other changes.. doubt that it affected confidence working though. |
@zaitsman yeah I tested it on the same images. |
@philipperemy so how do you expect it to give you a different value? I mean if your model is well fit then data from the training set is always 100%. You need to compare against OTHER images not in your dataset. |
@zaitsman @philipperemy i did not use training images and my confidence with @mshamash export was higher than when using detect.py |
Reference issues: #5157 , #343 , #7011
The current version of the export.py script outputs a CoreML model without NMS. This means that certain Vision APIs cannot be used with the model directly, as the output during inference is VNCoreMLFeatureValueObservation. The changes implemented here add a NMS layer to the CoreML output, so the output from inference is VNRecognizedObjectObservation. By adding NMS to the model directly, as opposed to later in code, the performance of the overall image/video processing is improved. This also allows use of the "Preview" tab in Xcode for quickly testing the model.
Default IoU and confidence thresholds are taken from the
--iou-thres
and--conf-thres
arguments during export.py script runtime. The user can also change these later by using a CoreML MLFeatureProvider in their application (see https://developer.apple.com/documentation/coreml/mlfeatureprovider).This has no effect on training, as it only adds an additional layer during CoreML export for NMS.
Based on code by @pocketpixels, with permission to make this PR: https://github.com/pocketpixels/yolov5/blob/better_coreml_export/models/coreml_export.py
🛠️ PR Summary
Made with ❤️ by Ultralytics Actions
🌟 Summary
Enhanced CoreML model export with NMS and metadata.
📊 Key Changes
CoreMLExportModel
to convert outputs to a more CoreML-friendly format.🎯 Purpose & Impact