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 cupyx.rsqrt #846

Merged
merged 4 commits into from Mar 1, 2018
Merged

Add cupyx.rsqrt #846

merged 4 commits into from Mar 1, 2018

Conversation

kmaehashi
Copy link
Member

This PR implements cupy.rsqrt as discussed in chainer/chainer#4092.

@beam2d
Copy link
Contributor

beam2d commented Dec 15, 2017

Is it safe to add rsqrt under the cupy namespace? I'm concerning the possibility of NumPy adding numpy.rsqrt in the future with a different interface (I believe it hardly occurs because it's more natural to support it as a ufunc, but we cannot guarantee that).

@kmaehashi
Copy link
Member Author

I followed the convention of how cupy.scatter_add is exposed (scatter_add is cupy-specific and placed directly under cupy namespace), but as you pointed out, rsqrt seems to have relatively higher risk of future conflict than scatter_add.
Providing it as cupy.ext.rsqrt may be better.

@unnonouno san, do you have any ideas?

@unnonouno
Copy link
Contributor

I prefer cupy.ext.rsqrt and also cupy.ext.scatter_add.

@kmaehashi
Copy link
Member Author

OK, I fixed to expose it as cupy.ext.rsqrt.

@kmaehashi kmaehashi changed the title Add cupy.rsqrt Add cupy.ext.rsqrt Dec 15, 2017
@niboshi
Copy link
Member

niboshi commented Dec 18, 2017

Is it safe to bring cupy.ext into a documented API?
There may be numpy.ext in the future.

@kmaehashi
Copy link
Member Author

Do you have a concern with the name ext, or (more generally) adding something under cupy namespace?

If the former is your concern, and we can't come up with any better name, we can ask "Do you have any plan to implement numpy.ext?" to numpy-discussion list. This at least notify NumPy dev team to know about us and the fact that we want to avoid future collision.

@niboshi
Copy link
Member

niboshi commented Dec 19, 2017

My concern is both.
Especially ext is relatively likely to be added to NumPy in the future.
While of course it's best to avoid using all such identifiers that could colide with NumPy, we may ignore the odds of numpy.fusion or numpy.memoize being introduced to NumPy.
But I don't think numpy.ext is the same (maybe subjective).

I think it's difficult (or impossible) for them to answer your question.
It should be our duty to avoid using such identifiers.

@kmaehashi
Copy link
Member Author

kmaehashi commented Dec 19, 2017

Especially ext is relatively likely to be added to NumPy in the future.

Honestly I was thinking of that but I didn't come up with the good name. How about special?

I think it's difficult (or impossible) for them to answer your question.
It should be our duty to avoid using such identifiers.

Yes, I completely agree with it. I'm also not expecting the answer or promise.
What I mean is that we can communicate with NumPy dev guys so that they (may possibly) keep a future conflict with us in their mind, not only for this case.
This is "done is better that doing nothing" approach. We have to pollute namespace anyway, and we are always responsible for it.

@beam2d
Copy link
Contributor

beam2d commented Dec 19, 2017

I feel special is more likely to conflict, observing scipy has this namespace.

The cleanest solution would be creating a completely new root namespace, like cupyext (or more shortly cupyx or sth like that). Isn't it possible?

@niboshi
Copy link
Member

niboshi commented Dec 21, 2017

I think they're good.

One concern is that cupyx reminds me of Cython extension .pyx, but maybe it's not relevant for users.

@niboshi niboshi added the cat:feature New features/APIs label Dec 21, 2017
@kmaehashi
Copy link
Member Author

In the weekly meeting we think it is good to have a separate package namespace (cupyx).
I'll update the PR accordingly.

@okuta okuta added the st:blocked-by-another-pr Blocked by another pull-request label Feb 2, 2018
@kmaehashi
Copy link
Member Author

Rebased to the latest master.

Copy link
Member

@okuta okuta left a comment

Choose a reason for hiding this comment

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

LGTM except comment.

def test_rsqrt(self, dtype):
# Adding 1.0 to avoid division by zero.
a = testing.shaped_arange((2, 3), numpy, dtype) + 1.0
out = cupy.ext.rsqrt(cupy.array(a))
Copy link
Member

Choose a reason for hiding this comment

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

cupyx.rsqrt ?

@kmaehashi kmaehashi changed the title Add cupy.ext.rsqrt Add cupyx.rsqrt Feb 19, 2018
@kmaehashi
Copy link
Member Author

Jenkins, test this please.

@okuta okuta assigned okuta and unassigned beam2d Mar 1, 2018
@okuta
Copy link
Member

okuta commented Mar 1, 2018

jenkins, test this please.

@codecov-io
Copy link

codecov-io commented Mar 1, 2018

Codecov Report

Merging #846 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #846   +/-   ##
=======================================
  Coverage   93.37%   93.37%           
=======================================
  Files         107      107           
  Lines        5807     5807           
=======================================
  Hits         5422     5422           
  Misses        385      385

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3de058a...f159f04. Read the comment docs.

@okuta okuta added this to the v4.0.0rc1 milestone Mar 1, 2018
@okuta
Copy link
Member

okuta commented Mar 1, 2018

Cloud you remove cupy/dlpack/a.out ?
And, please add rsqrt to document.

@okuta okuta added the st:awaiting-author Awaiting response from author label Mar 1, 2018
@kmaehashi
Copy link
Member Author

Oh, sorry. Force-pushed.

@kmaehashi kmaehashi removed the st:awaiting-author Awaiting response from author label Mar 1, 2018
@okuta okuta added the st:test-and-merge (deprecated) Ready to merge after test pass. label Mar 1, 2018
@okuta
Copy link
Member

okuta commented Mar 1, 2018

LGTM!

@okuta okuta merged commit 0dc2481 into cupy:master Mar 1, 2018
@kmaehashi kmaehashi deleted the add-rsqrt branch March 1, 2018 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:feature New features/APIs st:test-and-merge (deprecated) Ready to merge after test pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants