-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
exp2 extension #6197
exp2 extension #6197
Conversation
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.
About the tensorflow backend
I am not fully sure of the consequences of np_config.enable_numpy_behavior()
.... But enabling it file wide could lead to some unforeseen results in other functions.
I think its best if we use
return tf.math.pow(2, x)
instead of involving numpy into this at all.
So could you delete the np config import and behaviour setting and updte the implementation in the tesorflow backend.
I actually got an error that suggested using that when making use of numpy related functions, unless i used it the tests wouldn't pass. Switched for tf.math.pow as you suggested and it works as well, that's definitely better than relying on a file-wide change. |
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.
We actually dont need the tf.cast
. It should be removed.
It should just be return tf.math.pow(2,x)
. I think it should pass. Lmk when done and hopefully its ready
And the error for numpy related behaviour wast that tf's numpy experimental exp2 called x.astype internally with x being a tf tensor. This required np behaviour to be enabled. Bus fortunately we can steer clear from the whole thing.
I see, yeah the error was definitely related to the "astype" element, makes sense. |
Close #6196