add_int16_cast adds uint16 cast causing underflow of int16-compatible int32 value

[kasper0406]

The current add_int16_cast implementation has a side-effect in the should_cast_parameter method. It updates the target_dtype member when it is being called on different parameters. This target_dtype value is then used in the transform_op method, to add casts. The issue is that, in certain cases, the target_dtype does not get correctly reset between parameters, and unintended uint16 casts can occur!

The added test case will fail with:

E            ACTUAL: array([    0,     0,     0,     4, 65524,     4, 65515], dtype=int32)
E            DESIRED: array([  0,   0,   0,   4, -12,   4, -21], dtype=int32)

An easy fix would have been to reset the target_dtype state in every call to should_cast_parameter.
However, I believe this design is quite confusing, and likely to cause further issues, so I've gone ahead and refactored the code to make should_cast_parameter return an optional with the parameter's target dtype instead of having a side effect.

The transform_function_signatures method iterates the function inputs, and uses self.target_dtype to modify the inputs. This function did not use should_cast_parameter before as well, so depending on if it is called before/after the parameter conversion code, it may use a different self.target_dtype based on parameter ordering. In the new version, it will always use the default target_dtype. I am not sure what the correct behavior is here.

[TobyRoseman]

Thanks for the pull request. The fix looks good to me.

CI: https://gitlab.com/coremltools1/coremltools/-/pipelines/2189637393

[kasper0406]

@TobyRoseman the CI pipeline looks to be passing. Are you happy to get this merged?

[TobyRoseman]

@kasper0406 - before I merge, can you please address my one comment.

[kasper0406]

Sorry, I don't see any comments. Did you click "submit review"?

[TobyRoseman]

Sorry, I don't see any comments. Did you click "submit review"?

My bad. I've done it now. Thanks.

[kasper0406]

Cool, thanks! It's quite late here now, but I'll get it fixed over the weekend.

[TobyRoseman]

Updated CI: https://gitlab.com/coremltools1/coremltools/-/pipelines/2202537993

If this passes, I will merge this pull request.