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

add np.sign & np.abs #6

Merged
merged 3 commits into from Feb 20, 2020
Merged

Conversation

sangyx
Copy link
Collaborator

@sangyx sangyx commented Feb 19, 2020

Hi, I found a bug in np.positive and np.negative. The diff calculate in the source is incorrect:

register_diff(np.positive, lambda x: +x[1])
register_diff(np.negative, lambda x: -x[1])

If we suppose that x = [1, -1], then the y = np.positive(x)=[1, 1]. We can see that y = x if x > 0 and y=-x if x < 0. So the diff of y should be y=sign(x)=[1, -1]

@hameerabbasi
Copy link
Contributor

hameerabbasi commented Feb 19, 2020

Hello. I think you’re confusing what np.positive and np.negative do. They correspond to +x and -x, regardless of the value of x. So np.positive(-5) = -5 and np.negative(-5) = +5. Your derivative for np.sign is also slightly incorrect, unfortunately, as it’s undefined at x == 0. So it should be np.broadcast_to(np.where(x[0] == 0, float('nan'), 0), x[1].shape).

The second part is due to the product rule where the input is a function of the input. So you apply the chain rule f’(x) * d/dx g(f(x)) where f(x) is x[0], g(x) is np.sign(x) in this case, and f’(x) is x[1] here.

@hameerabbasi
Copy link
Contributor

What you’re suggesting is true for np.abs, however.

@sangyx
Copy link
Collaborator Author

sangyx commented Feb 19, 2020

Sorry for that. I can't guess the usage of the two functions and I should open an issue for this before the pr. I will try to complete the np.abs. But I think there is another problem. np.exp2 calculates 2**p for all p in the input array. But the code to calculate the derivative for it is :

register_diff(np.exp2, lambda x: x[1] * np.log(2) * np.exp(x[0]))

It should be:

register_diff(np.exp2, lambda x: x[1] * np.log(2) * np.power(2, x[0]))

@hameerabbasi
Copy link
Contributor

Ah, yes. That’s a mistake on my part, same for exp10. You can go ahead and fix that. I’d replace np.power(2, x) with np.exp2(x), but the rest seems good.

@sangyx
Copy link
Collaborator Author

sangyx commented Feb 19, 2020

Hi, when I complete the np.abs as:

register_diff(np.sign, lambda x: np.broadcast_to(np.where(x[0] == 0, float('nan'), 0), x[1].shape))
register_diff(np.absolute, lambda x: x[1] * np.where(np.sign(x[0]) == 0, float('nan'), np.sign(x[0])))

The diff of np.abs is fine. But the diff of np.sign encounter a problem:

PS F:\OneDrive - sjtu.edu.cn\Project\GSOC\udiff\src> python .\test.py
<DiffArray, name=unbound, arr=
[2. 3. 0.]
>
<DiffArray, name=unbound, arr=
[ 1. -1. nan]
>
Traceback (most recent call last):
  File ".\test.py", line 12, in <module>
    y = np.sign(x)
  File "F:\OneDrive - sjtu.edu.cn\Project\GSOC\udiff\src\udiff\_uarray_plug.py", line 65, in __ua_function__
    diff_arr = global_registry[a[0]](*a[1:], **kw)
  File "F:\OneDrive - sjtu.edu.cn\Project\GSOC\udiff\src\udiff\_builtin_diffs.py", line 32, in <lambda>
    register_diff(np.sign, lambda x: np.broadcast_to(np.where(x[0] == 0, float('nan'), 0), x[1].shape))
  File "D:\Python\lib\site-packages\unumpy\_multimethods.py", line 105, in f
    return globals()[name](self, other)
  File "F:\OneDrive - sjtu.edu.cn\Project\GSOC\udiff\src\udiff\_uarray_plug.py", line 65, in __ua_function__
    diff_arr = global_registry[a[0]](*a[1:], **kw)
KeyError: <ufunc 'equal'>

But I don't know how to complete the diff of np.equal.

@hameerabbasi
Copy link
Contributor

Hmm. Try x[0].arr.

@sangyx sangyx changed the title fix np.positive & np.negative add np.sign & np.abs Feb 19, 2020
@sangyx
Copy link
Collaborator Author

sangyx commented Feb 19, 2020

Thx, the pr have committed.

Copy link
Contributor

@hameerabbasi hameerabbasi left a comment

Choose a reason for hiding this comment

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

One small comment, then I think this is good to go.

register_diff(np.negative, lambda x: -x[1])
register_diff(np.conj, lambda x: np.conj(x[1]))
register_diff(np.absolute, lambda x: x[1] * np.where(np.sign(x[0]) == 0, float('nan'), np.sign(x[0])))
register_diff(np.positive, lambda x: x[1] * np.sign(x[0]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you revert these two lines?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think you mean the np.negative and np.positive?

@hameerabbasi hameerabbasi merged commit e23163c into Quansight-Labs:master Feb 20, 2020
@hameerabbasi
Copy link
Contributor

Thanks @sangyx. This is in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants