Fix precision loss in CropWindowGenerator #1735
Conversation
JanuszL
requested review from
jantonguirao,
szalpal,
klecki,
mzient and
awolant
|
!build |
|
CI MESSAGE: [1129258]: BUILD STARTED |
| int64_t slice_end; | ||
| // special case - minimize the floating point error by multiplying only once after sum | ||
| if (normalized_anchor_ && normalized_shape_) { | ||
| slice_end = static_cast<int64_t>((anchor_val + shape_val) * shape[dim]); |
There was a problem hiding this comment.
In my opinion, we should be rounding, not truncating. This would make relative slice flip-invariant (now it's not).
There was a problem hiding this comment.
...and the loss of precision, if any, is rather marginal. If we care for extreme cases (slicing huge 1D objects, for example), this should be a double anyway.
There was a problem hiding this comment.
It is marginal but still there is a divergence between implementations. So I wanted to make that consistent. Will switch to round.
| if (normalized_shape_) { | ||
| shape_val *= shape[dim]; | ||
| } | ||
| slice_end = static_cast<int64_t>(anchor_val + shape_val); |
|
CI MESSAGE: [1129364]: BUILD STARTED |
|
CI MESSAGE: [1129454]: BUILD FAILED |
|
CI MESSAGE: [1129454]: BUILD STARTED |
1 similar comment
|
CI MESSAGE: [1129454]: BUILD STARTED |
|
CI MESSAGE: [1129258]: BUILD PASSED |
|
CI MESSAGE: [1129454]: BUILD PASSED |
| @@ -238,24 +238,25 @@ def slice_func_helper(axes, axis_names, layout, normalized_anchor, normalized_sh | |||
| full_slice_anchor[axis] = slice_anchor[idx] | |||
| full_slice_shape[axis] = slice_shape[idx] | |||
|
|
|||
| #std::round has different behaviour than np.round so manually ad 0.5 and truncate to int | |||
There was a problem hiding this comment.
| #std::round has different behaviour than np.round so manually ad 0.5 and truncate to int | |
| #std::round has different behaviour than np.round so manually add 0.5 and truncate to int |
;)
| if (normalized_shape_) { | ||
| shape_val *= shape[dim]; | ||
| } | ||
| slice_end = std::llround(anchor_val + shape_val); |
There was a problem hiding this comment.
This is not correct, especially for really big data. Cast to double or calculate slice_end using integer start/end in respective if branches.
|
!build |
|
CI MESSAGE: [1134768]: BUILD STARTED |
|
CI MESSAGE: [1134768]: BUILD FAILED |
- CropWindowGenerator computes anchor and shapes as a rel_anch * shape, rel_shape * shape. To be more accurate it should be rel_anch * shape, (rel_anch + rel_shape) * shape - (rel_anch * shape) - adds a special case when anchors and shapes are relative. When only one or none is then the old code path is used - adds rounding in place of truncation - updates tests - uses doubles in place of floats to improve precision Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
|
!build |
|
CI MESSAGE: [1135044]: BUILD STARTED |
|
CI MESSAGE: [1135044]: BUILD PASSED |
Signed-off-by: Janusz Lisiecki jlisiecki@nvidia.com
Why we need this PR?
Pick one, remove the rest
What happened in this PR?
Fill relevant points, put NA otherwise. Replace anything inside []
adds a special case when anchors and shapes are relative. When only one or none is then the old code path is used
adds rounding in place of truncation
slice_attr
NA
Updated tests are run on CI
NA
JIRA TASK: [NA]