Skip to content
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

convert_element_type: don't canonicalize old_dtype #6018

Merged
merged 1 commit into from
Mar 15, 2021

Conversation

jakevdp
Copy link
Collaborator

@jakevdp jakevdp commented Mar 10, 2021

Related to #5982 and #6014. Canonicalizing the input type can lead to the primitive not being bound, because old_dtype doesn't reflect the actual value of the input dtype.

The added test fails on master, and passes on this branch. This is orthogonal to the change in #6014; they both fix similar issues.

Copy link
Collaborator

@mattjj mattjj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice fix. I was at first worried that this would change how we transpose things, but as Jake explained to me offline, this old_dtype value isn't passed into the primitive bind (though it used to be); it's just used in this traceable wrapper.

@jakevdp jakevdp added the pull ready Ready for copybara import and testing label Mar 11, 2021
@copybara-service copybara-service bot merged commit 80966fe into jax-ml:master Mar 15, 2021
@jakevdp jakevdp deleted the conv-elem-type branch March 15, 2021 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes pull ready Ready for copybara import and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants