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

Jump to bottom

Add Singular Value Decomposition to cupy #2481

Merged
merged 7 commits into from May 2, 2017
Merged

Add Singular Value Decomposition to cupy #2481

merged 7 commits into from May 2, 2017

Conversation

rezoo
Copy link
Member

@rezoo rezoo commented Mar 30, 2017

Merge after #2412.
This PR aims to add the Singular Value Decomposition (SVD) to cupy.

Note: this PR is based on #1402.

@unnonouno
Copy link
Member

Please rebase the master

@unnonouno unnonouno self-assigned this Apr 14, 2017
@toslunar
Copy link
Member

I got

>>> cupy.linalg.svd(cupy.array([[1]], dtype=cupy.float32))
(array([[-4765.43359375]], dtype=float32), array([ 1.], dtype=float32), array([[ 1.]], dtype=float32))
>>> cupy.linalg.svd(cupy.array([[1]], dtype=cupy.float64))
(array([[ 0.]]), array([ 1.]), array([[ 1.]]))

while

>>> cupy.linalg.svd(cupy.array([[1,0],[0,1]], dtype=cupy.float32))
(array([[ 1.,  0.],
       [ 0.,  1.]], dtype=float32), array([ 1.,  1.], dtype=float32), array([[ 1., -0.],
       [ 0.,  1.]], dtype=float32))

is OK.

@rezoo
Copy link
Member Author

rezoo commented Apr 17, 2017

I rebased this branch.

@rezoo
Copy link
Member Author

rezoo commented Apr 17, 2017

@toslunar Thanks for reporting the bug. It seems that this unexpected behavior arises from (s|d)gesvd in cusolver, but I have no idea whether it is better to deal with this problem by ours or not.

unnonouno
unnonouno requested changes Apr 17, 2017
View reviewed changes
Copy link
Member

@unnonouno unnonouno left a comment

Choose a reason for hiding this comment

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

Looks good! Check my minor comments.

cupy/linalg/decomposition.py Outdated
# Remark 4: Remark 2 is removed since cuda 8.0 (new!)
n, m = a.shape
if m >= n:
x = a.astype(dtype, copy=True)
Copy link
Member

Choose a reason for hiding this comment

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

Is copy required?

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact there is no need for adding copy = True as argument since copy in astype is True by default, but I just wanted to clarify a cupy from a to x always occurs.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, does copy=False work correctly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Through reading the following document, I noticed that in this case copy=False works correctly.
https://docs.scipy.org/doc/numpy/reference/generated/numpy.ndarray.astype.html

Copy link
Member Author

@rezoo rezoo Apr 27, 2017 •
edited

Choose a reason for hiding this comment

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

Through reading the document, in this case copy=False seems to work correctly.
However, when I tried to use copy=False, the result becomes unstable and tests failed... Do you know any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Do you need to call ascontiguousarray?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I confirmed that it works well with a pair of copy=False and ascontiguousarray.

cupy/linalg/decomposition.py Outdated
u_ptr, vt_ptr = 0, 0 # Use nullptr
s = cupy.empty(mn, dtype=dtype)
handle = device.get_cusolver_handle()
devInfo = cupy.empty(1, dtype=numpy.int32)
Copy link
Member

Choose a reason for hiding this comment

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

Use snake case dev_info

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

cupy/linalg/decomposition.py Outdated
devInfo = cupy.empty(1, dtype=numpy.int32)
if compute_uv:
if full_matrices:
jobu, jobvt = ord('A'), ord('A')
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to use two variables? They hold the same value in all cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do not need to use two variables in this case, but I just wanted to clarify that jobu and jobvt are always the same.

    if compute_uv:
        job = ord('A') if full_matrices else ord('S')
    else:
        job = ord('N')

is also OK.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

tests/cupy_tests/linalg_tests/test_decomposition.py
@unittest.skipUnless(
cuda.cusolver_enabled, 'Only cusolver in CUDA 8.0 is supported')
@testing.gpu
class TestSVD(unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test case for invalid arguments for _assert_rank2?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

tests/cupy_tests/linalg_tests/test_decomposition.py Outdated
a_gpu = cupy.asarray(array, dtype=dtype)
result_cpu = numpy.linalg.svd(a_cpu, full_matrices=self.full_matrices)
result_gpu = cupy.linalg.svd(a_gpu, full_matrices=self.full_matrices)
for b_cpu, b_gpu in zip(result_cpu, result_gpu):
Copy link
Member

Choose a reason for hiding this comment

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

Use six.moves.zip

Copy link
Member

Choose a reason for hiding this comment

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

Check self.assertEqual(len(result_cpu), len(result_gpu))

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

tests/cupy_tests/linalg_tests/test_decomposition.py Outdated
result_gpu = cupy.linalg.svd(a_gpu, full_matrices=self.full_matrices)
for b_cpu, b_gpu in zip(result_cpu, result_gpu):
# Use abs to support an inverse vector
cupy.testing.assert_allclose(
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to fix numpy_cupy_allclose to support tuples?

Copy link
Member Author

Choose a reason for hiding this comment

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

In near future, yes, but currently it is better to manually check the tuples.

Copy link
Member

Choose a reason for hiding this comment

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

OK

@rezoo
Copy link
Member Author

rezoo commented Apr 27, 2017

@unnonouno I fixed all the points you have mentioned. Could you check them?

unnonouno
unnonouno reviewed Apr 29, 2017
View reviewed changes
Copy link
Member

@unnonouno unnonouno left a comment

Choose a reason for hiding this comment

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

I wrote comments. If it is difficult to fix it, please add TODO comment about the phenomena, and then i'll merge it.

@rezoo
Copy link
Member Author

rezoo commented May 1, 2017

I fixed the problem of copy=False.

@unnonouno unnonouno merged commit 9fad2f9 into chainer:master May 2, 2017
@unnonouno
Copy link
Member

OK, LGTM!

@unnonouno unnonouno added CuPy Related to CuPy. cat:feature Implementation that introduces new interfaces. labels May 2, 2017
@unnonouno unnonouno added this to the v1.24.0 milestone May 2, 2017
@rezoo rezoo deleted the linalg-svd branch May 2, 2017 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@unnonouno unnonouno unnonouno requested changes

Assignees

@unnonouno unnonouno

Labels
cat:feature Implementation that introduces new interfaces. CuPy Related to CuPy.
Projects
None yet
Milestone
v1.24.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rezoo @unnonouno @toslunar