Fix avgPool3d #7133
Fix avgPool3d #7133
Conversation
| } | ||
| setOutput(${returnValue}); |
There was a problem hiding this comment.
setOutput(${returnValue}); should happen out of all loops and just before main returns. Originally, it is within for (int wD = 0; wD < ${effectiveFilterDepth};'s loop
| roundingMode); | ||
| } | ||
| } | ||
| return outShape; |
There was a problem hiding this comment.
Adds two changes in computeOutputShape4D function:
- add a if-branch to make sure all dimensions of outShape are non-negative.
- outputDepths, outputRows, outputCols should be computed from the corresponding strides and filterSize respectfully.
| @@ -496,6 +495,10 @@ function get3DPadAndOutInfo( | |||
| let outHeight: number; | |||
| let outWidth: number; | |||
|
|
|||
| if (pad === 'valid') { | |||
| pad = 0; | |||
| } | |||
There was a problem hiding this comment.
If pad is 'valid', it should have the same result as pad = 0.
By the way, the roundingMode for this case is supposed to be 'truncate', instead of 'ceil', referring to tensorflow.
There was a problem hiding this comment.
great work, thank you Lin!
Reviewable status:
complete! 2 of 1 approvals obtained (waiting on @Linchenn)
tfjs-backend-webgl/src/pool_gpu.ts line 455 at r1 (raw file):
Previously, Linchenn wrote…
setOutput(${returnValue});should happen out of all loops and just beforemainreturns. Originally, it is withinfor (int wD = 0; wD < ${effectiveFilterDepth};'s loop
good catch!
tfjs-core/src/ops/avg_pool_3d.ts line 91 at r7 (raw file):
util.assert( (typeof strides === 'number' && strides > 0) || (typeof strides === 'object' && strides[0] > 0 && strides[1] > 0 &&
should we use Arrays.isArray()?
tfjs-core/src/ops/conv_util.ts line 500 at r6 (raw file):
Previously, Linchenn wrote…
If pad is 'valid', it should have the same result as
pad = 0.By the way, the roundingMode for this case is supposed to be 'truncate', instead of 'ceil', referring to tensorflow.
thanks for checking TF implementation.
There was a problem hiding this comment.
Thank you!
Reviewable status:
tfjs-core/src/ops/avg_pool_3d.ts line 91 at r7 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
should we use Arrays.isArray()?
Done.
| // Use `max(count, 1.0)` instead of `count` in case count === 0.0. | ||
| // If count === 0.0, `avgValue` is always 0.0 and we change `count`'s | ||
| // value to avoid dividing zero. | ||
| returnValue = `avgValue / max(count, 1.0)`; |
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks @Linchenn . Just curious, is L124 returnValue = avgValue / count; missed for change?
And another question is that it seems that it's not possible to happen that count is zero in
updateSnippet will increase count. Will there be a situation that the filter window is totally no overlap with the input window?
There was a problem hiding this comment.
Great catch, thanks!
If padding >= filter size, this would happen, as #7122.
There was a problem hiding this comment.
LGTM. Thanks for your explanation.
Can you help to cover the webgpu change in this PR since you already find the right place? :) And it will be great if you add a similar case as #7122 to file avg_pool_3d_test.ts?
BUG * fix webgl * rename * add strides check * Update conv_util.ts * reduce valid * lint * reduce * add tests * isArray * Update pool_gpu.ts * Update pool2d_webgpu.ts * Update avg_pool_3d_test.ts * Update avg_pool_3d_test.ts * skip tests for node
This PR fixes a couple of issues for avgPool3d for Core, CPU-backend and GPU-backend:
padto a number, the result of tf.avgPool3d is abnormal. #7122To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is