-
Notifications
You must be signed in to change notification settings - Fork 647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 2153/ Refactor PyTorch 64-Bit Data Type Conversion #2153 #2171
Issue 2153/ Refactor PyTorch 64-Bit Data Type Conversion #2153 #2171
Conversation
Thanks @teelrabbit for the proposed fix. We will need to also fix where all those "number to dtype" mapping are used, e.g. ops.py |
Makes sense. I'll do some testing and see I can use the current solution across all instances of where the mapping is used 🤙🏻 |
Made some changes to the occurrences of NUM_TO_TORCH_DTYPE to use "dtype_to_32bit". Can you comfirm if this is what you intended by "number to dtype". https://pastes.dev/2oAOqJeDM1 @YifanShenSZ ' np_type = nptype_from_builtin(target_dtype.dtype)
dtype = NUMPY_DTYPE_TO_TORCH_NUM[np_type] dtype = NUMPY_DTYPE_TO_TORCH_NUM[np_type]
torch_dtype = NUM_TO_TORCH_DTYPE[dtype] torch_dtype = dtype_to_32bit(dtype)
if isinstance(_input, Var) and _input.can_be_folded_to_const(): if isinstance(_input, Var) and _input.can_be_folded_to_const():
# numpy -> torch -> torch cast -> numpy # numpy -> torch -> torch cast -> numpy
# This path is needed to use the mapping of passed in dtypes to torch dtypes. # This path is needed to use the mapping of passed in dtypes to torch dtypes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last minor revision needed, overall LGTM: As CI suggests, we should still keep the number keys 4
and 7
, and update the corresponding values to 64-bit dtypes
Sorry about the review delay, we were on a change freeze due to 7.2 release 🙏 Many thanks for polishing coremltools!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! CI ✅ Many thanks for contributing to coremltools!
|
Also this issue should be closed out #2153 (comment) |
Made changes so it uses a function dtype_to_32bit that converts 64-bit data types to 32-bit data types.