Conversation
|
@sl-sergei Could you look at CI logs and tests results. |
Sure |
sl-sergei
left a comment
There was a problem hiding this comment.
It seems that function convertFp16 provides incorrect weights/biases, all other networks without Conv layers work fine in tests with FP16. More investigation required
| if (bestKernelConfig.empty()) | ||
| return false; | ||
| return convolve(bottom, top, weight, (use_half_) ? bias_half : bias, numImages, bestKernelConfig); | ||
| return convolve(bottom, top, (use_half_) ? weights_half : weight, (use_half_) ? bias_half : bias, numImages, bestKernelConfig); |
There was a problem hiding this comment.
Looks like OpenCL kernels are still called properly (without this patch).
There are several other places with handling this in this file: 1 2 3
IMHO, it is a coding issue.
There is no reason to pass FP32 weight/bias to this implementation.
FP16 should be passed instead by caller (convolution_layer.cpp).
Also it should avoid double conversion FP16->FP32->FP16 hacks like this.
|
@alalek Can you please provide the details about testing machine configuration for |
|
@sl-sergei I restarted CI builds, let's check again. |
|
Replaced by accurate fix in #19115 |
|
Does it mean we can load a "normal" (fp32) onnx and run fp16 inference? |
relates #18457
Pull Request Readiness Checklist
This PR doesn't fix #18457, but during the debug, I found that
weights_halfis never used even it has been converted fromweightI confirmed the this PR doesn't harm the tests.
perf_dnnfails as same as before, and some tests fails even before from this PR, so nothing gets worse.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.