openvino icon indicating copy to clipboard operation
openvino copied to clipboard

Convert with clamp when do input precision conversion

Open sgbihu opened this issue 2 months ago • 3 comments

Details:

  • The Slice operator with int64 input will be inserted convert operator to convert int64 -> int32 image

  • The current Convert will convert int64_min to 0, which creates an accuracy issue.

Tickets:

sgbihu avatar Nov 05 '25 13:11 sgbihu

@mryzhov Could you check if ConvertPrecision should insert Clamp operator (or have special mode) in cases where simple value conversion may produce incorrect value?

praasz avatar Nov 06 '25 10:11 praasz

@mryzhov Could you check if ConvertPrecision should insert Clamp operator (or have special mode) in cases where simple value conversion may produce incorrect value?

I have reviewed all comments, and I know this solution is not good. So I want to get suggestions from reviewer. I list all tests that I have tried.

  • The Clamp operator only supports float type for min/max, https://docs.openvino.ai/2025/documentation/openvino-ir-format/operation-sets/operation-specs/activation/clamp-1.html. That will make the INT32_MIN lose data when doing conversion.
  • I used the Maximum and Minimum operators to implement the function, but the ConvertPrecision executes twice. And it reports some errors

sgbihu avatar Nov 07 '25 02:11 sgbihu

A rather philosophical question: if we suppose that Convert works incorrectly in some cases (btw do you know what are the cases? Can you formulate the particular scenarios?), shouldn't we adjust the Convert behavior in the first place to avoid essentially the manual conversion? @praasz @mryzhov

CuriousPanCake avatar Nov 13 '25 10:11 CuriousPanCake

I suggest merging this PR first: https://github.com/openvinotoolkit/openvino/pull/33250 These are the transformations changes that can be done independently. You'd just need to rebase.

CuriousPanCake avatar Dec 15 '25 16:12 CuriousPanCake

I suggest merging this PR first: #33250 These are the transformations changes that can be done independently. You'd just need to rebase.

It looks like the separate PR will not pass the CI on its own, so we'll have to include the transformations changes here. @sgbihu could you please revert the revert and make the corresponding changes here, please?

CuriousPanCake avatar Dec 16 '25 10:12 CuriousPanCake