Skip to content
This repository has been archived by the owner on Oct 31, 2023. It is now read-only.

Add helper function "cv2_utils.findContours" to maintain version compatibility between versions 3 and 4 #354

Merged
merged 1 commit into from
Jan 21, 2019

Conversation

aaronlelevier
Copy link
Contributor

Summary

Small PR to fix the version compatibility problem with cv2.findContours returning either 2 or 3 values depending on the cv2 version.

I replaced all occurrences of cv2.findContours with the helper.

I put the code in utils/ dir. Seemed like the best place for it, but please advise.

Testing

I tested locally by installing my branch locally using:

pip install -U git+https://github.com/aaronlelevier/maskrcnn-benchmark.git@train

Then ran the demo/Mask_R-CNN_demo.ipynb and it worked as expected, even though my cv2 version is opencv-python==4.0.0.21, so this is now compatible with either version.

References

fixes #336

#339 also referenced this error but the issue is already closed

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jan 19, 2019
Copy link
Contributor

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

What were the changes in demo/Mask_R-CNN_demo? I couldn't see them as there is way too many additions. I suppose it's not intended?

requirements.txt Outdated
@@ -0,0 +1,13 @@
requests
Copy link
Contributor

Choose a reason for hiding this comment

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

When is the requirements file used? Let's maybe keep the diff minimal only with things that are related to the opencv fix?

@fmassa
Copy link
Contributor

fmassa commented Jan 21, 2019

and thanks for the PR by the way!

@aaronlelevier
Copy link
Contributor Author

Okay, I removed the other files / commits, so this PR is now only related to the cv2.findCountours change.

I meant to change branches and was still working on this branch, and didn't realize that it was also my PR branch!

All, should be good now. Please lmk if you have any other feedback? I did see the comment to support cv2 versions 1-4 in #336 's suggested snippet , but I thought maybe only versions 3-4 is relevant, so my code is for that.

Thanks

Copy link
Contributor

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@fmassa fmassa merged commit af132c5 into facebookresearch:master Jan 21, 2019
@aaronlelevier aaronlelevier deleted the train branch January 21, 2019 17:14
kris-singh pushed a commit to kris-singh/maskrcnn-benchmark that referenced this pull request Jan 22, 2019
Lyears pushed a commit to Lyears/maskrcnn-benchmark that referenced this pull request Jun 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants