-
Notifications
You must be signed in to change notification settings - Fork 358
Add power method and test to numpy backend. #849
Conversation
Codecov Report
@@ Coverage Diff @@
## master #849 +/- ##
=========================================
Coverage ? 98.37%
=========================================
Files ? 129
Lines ? 22124
Branches ? 0
=========================================
Hits ? 21764
Misses ? 360
Partials ? 0
Continue to review full report at Codecov.
|
So right @mganahl @Thenerdstation I think we need to make a ruling on what to do in this case:
|
I think taking the absolute value of the input is not the way to go since this is actually more surprising than NaN - at least NaN will make your program crash. |
i think we should stick to whatever the backends are doing. |
We're gonna add it to all of them, just not in the same PR
…On Sun, Oct 11, 2020, 15:04 Martin Ganahl ***@***.***> wrote:
i think we should stick to whatever the backends are doing.
Why only add power to numpy and not the other backends as well?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#849 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAINHBXIHRUOYP7K5AGY2DLSKH6TPANCNFSM4SGUB67Q>
.
|
If we stick to what the backends are doing, the behaviour of our programs
will change as the backend does, which I thought we were trying to avoid.
Is that ok do you think?
…On Sun, Oct 11, 2020, 15:26 Adam Lewis ***@***.***> wrote:
We're gonna add it to all of them, just not in the same PR
On Sun, Oct 11, 2020, 15:04 Martin Ganahl ***@***.***>
wrote:
> i think we should stick to whatever the backends are doing.
> Why only add power to numpy and not the other backends as well?
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <#849 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAINHBXIHRUOYP7K5AGY2DLSKH6TPANCNFSM4SGUB67Q>
> .
>
|
In case this is helpful to have on hand: TensorFlow's power method throws an error when a negative float is raised to a non-integer power: PyTorch's power method does the same as Numpy's and silently returns nan values: |
Numpy returns nan because raising a negative float to a fractional power changes the dtype to complex. np.power(complex(-1), 0.1) works fine. As of @wkostuch comment, it seems that all backends are behaving similarly in that respect, so I'd just leave it that way. Otherwise we will have to violate type preservation (which in numpy will lead to propagating dtypes), which can cause a whole bunch of issues itself. |
Ok, SGTM
…On Sun, Oct 11, 2020, 17:30 Martin Ganahl ***@***.***> wrote:
If we stick to what the backends are doing, the behaviour of our programs
will change as the backend does, which I thought we were trying to avoid.
Is that ok do you think?
Numpy returns nan because raising a negative float to a fractional power
changes the dtype to complex. np.power(complex(-1), 0.1) works fine.
As of @wkostuch <https://github.com/wkostuch> comment, it seems that all
backends are behaving similarly in that respect, so I'd just leave it that
way. Otherwise we will have to violate type preservation (which in numpy
will lead to propagating dtypes), which can cause a whole bunch of issues
itself.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#849 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAINHBVTFK6BUPAGD47FS3TSKIPXZANCNFSM4SGUB67Q>
.
|
@mganahl, I was hoping to get around to them both this weekend. I've been swamped with school and exams the last little while so I haven't had time to do this. Sorry 'bout that! |
I'm a little unsure about the test method for power.
Due to how np.power works, if you have a negative base and raise that to a non-integer power you receive np.nan. However, since the tests use backend.randn, it's very likely to get a negative value as a base and then try to raise that to a non-integer power, resulting in a runtime warning.
The current "solution" I have is to take the absolute value of the base tensor, ensuring the (-x)**(not-an-integer) situation never occurs. Is there a better way to deal with the np.power quirk or does this suffice?
Appreciate the feedback.