-
Notifications
You must be signed in to change notification settings - Fork 4
Update Scenedetection #70
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
|
Please can you do a new PR but where these changes are in a new scene detection file e.g. submit your changes as svm_poly3 (or other name of your choice) ? |
angrave
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.
Please see my line-based comments - (basically more comments needed to explain why some choices were made)
Also please can you do a new PR but where these changes are in a new scene detection file e.g. submit your changes as svm_poly3 (or other name of your choice) ?
i.e. I want to be able to be able switch between the current and your new methods.
| M, inliers = cv2.estimateAffinePartial2D(src_pts, dst_pts, method=cv2.RANSAC) | ||
|
|
||
| inlier_ratio = np.sum(inliers) / len(inliers) | ||
| # Arbitrary threshold |
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.
Some comments would be useful.
Where did this value come from? Are there any links to web resources that would help me/someone tweak this i the future? What is a reasonable range? What happens if it is too large too small?
The value should at least be in a variable to highlight it
| if m.distance < 0.7 * n.distance: | ||
| good.append(m) | ||
|
|
||
| if len(good) < 100: |
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.
Arbtirary constant should be in a variable. Documentation... Valid range? Web resources? When should it be increased? decreased?
|
|
||
| matches = flann.knnMatch(des1, des2, k=2) | ||
|
|
||
| # Ratio test as per Lowe's paper |
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.
Please cite e.g. with DOI or web URL so people can find it
| # print(f"No descriptors found (des1={des1 is None}, des2={des2 is None}) at index {index}") | ||
| return False | ||
|
|
||
| # FLANN parameters |
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 few notes would be useful here, includingwhat these parameters do
| # Create target | ||
| crop_image = ref[img_y:img_y+img_h, img_x:img_x+img_w] | ||
|
|
||
| # Initiate SIFT detector |
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 web link would be usesful, plus a comment what this detector does
| return result | ||
|
|
||
| def find_match(curr, ref, width, height, index): | ||
| # Defining region of search |
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.
Explain these choices
fea3a5f to
45e0dd8
Compare
angrave
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.
LGTM
Add function to handle scrolling-related false positives in scene detection