Fix Robertson Calibration NaN Bug#20054
Conversation
|
@danielenricocahall Please take a look on failed builds (you may start from "debug" local build) |
modules/photo/test/test_hdr.cpp
Outdated
| Mat response_no_nans = response.clone(); | ||
| patchNaNs(response_no_nans); | ||
| // since there should be no NaNs, original response vs. response with NaNs patched should be identical | ||
| bool isEqual = (sum(response != response_no_nans) == Scalar(0,0,0)); |
There was a problem hiding this comment.
norm(response, response_no_nans, NORM_INF) == 0.0 ?
There was a problem hiding this comment.
That's smarter - let me do that. Thanks!
There was a problem hiding this comment.
NORM_INF doesn't properly handle NaNs.
There is still a problem:
EXPECT_EQ(0.0, cv::norm(response, response_no_nans, NORM_L2));
Results to this error message (Linux Debug):
Expected equality of these values:
0.0
Which is: 0
cv::norm(response, response_no_nans, NORM_L2)
Which is: -nan
[ FAILED ] Photo_CalibrateRobertson.bug_18180 (1789 ms)
There was a problem hiding this comment.
Ah I figured out the issue, I had to move the epsilon back. Addressed. Thank you!
modules/photo/test/test_hdr.cpp
Outdated
| glob(string(cvtest::TS::ptr()->get_data_path()) + "hdr/exposures/bug_18180/*.jpg", fn, false); | ||
| for (auto & i : fn) | ||
| images.push_back(imread(i)); |
There was a problem hiding this comment.
Please avoid using glob() in tests. Use fixed input instead.
46543be to
c9feccc
Compare
I think all issues are resolved - please feel free to re-review at earliest convenience. |
fix windows build warnings fix vector type for tests update tests make threshold float address test comments fix tests and move epsilon again
db7479f to
434daba
Compare
|
thank you! 👍 |
modules/photo/src/merge.cpp
Outdated
| wsum += times.at<float>((int)i) * times.at<float>((int)i) * w; | ||
| wsum += times.at<float>((int)i) * times.at<float>((int)i) * (w + DBL_EPSILON); | ||
| } | ||
| result = result.mul(1 / wsum); |
There was a problem hiding this comment.
Patching wsum doesn't look valid.
Patch divide operation instead (Scalar::all() is necessary as we work with multi-channel data):
result = result.mul(1 / (wsum + Scalar::all(DBL_EPSILON)));
There was a problem hiding this comment.
@alalek Ah good catch. It technically works, but a warning pops up:
assign OpenCV/MatExpr: processing of multi-channel arrays might be changed in the future: https://github.com/opencv/opencv/issues/16739
Addressed the issue and actually moved the epsilon to where the division occurs. Thank you!
…-calibration-bug Fix Robertson Calibration NaN Bug * add epsilon value for numerical stability in robertson merge * update test to use range based for loop * add comment to test * move the epsilon * address test comments fix windows build warnings fix vector type for tests update tests make threshold float address test comments fix tests and move epsilon again * use scalar::all, move epsilon, and remove print
Merge with extra: opencv/opencv_extra#873
Addresses #18180. The issue seemed to be with numerical stability when
w_summatrix contained zeroes. Should be merged in conjunction with opencv/opencv_extra#873, which contains test data taken from original issue.Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.