-
Notifications
You must be signed in to change notification settings - Fork 15
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 tests for chessboard corner detection #52
base: master
Are you sure you want to change the base?
add tests for chessboard corner detection #52
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #52 +/- ##
=======================================
Coverage 18.75% 18.75%
=======================================
Files 3 3
Lines 32 32
=======================================
Hits 6 6
Misses 26 26 ☔ View full report in Codecov by Sentry. |
Super weird. I noticed that just |
(@timholy, you asked me to ping you directly) |
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.
Thanks for the PR! It mostly looks good. I've left a few code comments, but one overall comment:
together with the maintainers of this repo
This repo has no maintainers. There are people like me who have merge privileges via JuliaImages, but I don't know or use OpenCV. It would be great if this package got a real maintainer.
I don't know how to fix the "must be initialized" bug, sadly.
img = load(file) | ||
gry = img[1:1, :, :] | ||
cv_n_corners = OpenCV.Size{Int32}(n_corners...) | ||
_cv_corners = OpenCV.Mat(Array{Float32}(undef, 2, 1, prod(n_corners))) |
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.
Out of curiosity, could this be declared OpenCV.Mat(Matrix{Tuple{Float32,Float32}}(undef, n_corners))
? And then avoid the reshaping/eachslice
below?
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.
Currently, OpenCV.Mat
can only accept 3D arrays, not matrices:
MethodError: no method matching OpenCV.Mat(::Matrix{Tuple{Float32, Float32}})
The type `OpenCV.Mat` exists, but no method is defined for this combination of argument types when trying to construct it.
Closest candidates are:
OpenCV.Mat(::AbstractArray{T, 3}) where T<:Union{Float32, Float64, Int16, Int32, Int8, UInt16, UInt8}
@ OpenCV ~/.julia/artifacts/dcc70861ed0ffc5427898f6aeb436620f33fa02f/OpenCV/src/Mat.jl:13
So that's not an option.
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.
You should be able to do
corners = Matrix{Tuple{Float32,Float32}}(undef, n_corners)
ret, cv_corners = OpenCV.findChessboardCorners(gry, cv_n_corners, OpenCV.Mat(reinterpret(reshape, Float32, corners)), 0)
@assert ret "Failed to detect any corners!"
return corners
if you think that's a little bit clearer/cleaner. It certainly makes inference's job easier.
Co-authored-by: Tim Holy <[email protected]>
I agree and understand. I'm not even sure there's any need to know much about OpenCV to fix this. As I've noticed elsewhere, |
This is my first PR in hopes to add a full test-suit for the opencv camera calibration functions. My secret motive is that I've found that
OpenCV.calibrateCamera
randomly fails (i.e. return nonsensical values), and I hope to unearth the issue together with the maintainers of this repo.