apple
`add_int16_cast` adds uint16 cast causing underflow of int16-compatible int32 value
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.
Thanks for the pull request. The fix looks good to me.
CI: https://gitlab.com/coremltools1/coremltools/-/pipelines/2189637393
@TobyRoseman the CI pipeline looks to be passing. Are you happy to get this merged?
Sorry, I don't see any comments. Did you click "submit review"?
My bad. I've done it now. Thanks.
Cool, thanks! It's quite late here now, but I'll get it fixed over the weekend.
Updated CI: https://gitlab.com/coremltools1/coremltools/-/pipelines/2202537993
If this passes, I will merge this pull request.
