Make SVD overwrite temporary array x #2277

danielhanchen · 2019-06-26T12:51:23Z

@pentschev
Regarding our convo.
SVD can now auto overwrite the temporary array x. Can improve performance possibly (haven't tested yet).
Should support slightly larger data sizes also.

@pentschev

@pentschev SVD can now auto overwrite the temporary array x. Can improve performance possibly (haven't tested yet).

hvy · 2019-06-27T04:55:37Z

cupy/linalg/decomposition.py

@@ -290,12 +309,14 @@ def svd(a, full_matrices=True, compute_uv=True):
        raise linalg.LinAlgError(
            'Parameter error (maybe caused by a bug in cupy.linalg?)')

-    # Note that the returned array may need to be transporsed
+    # Note that the returned array may need to be transprosed


A nitpick but a typo.

Suggested change

# Note that the returned array may need to be transprosed

# Note that the returned array may need to be transposed

hvy · 2019-06-27T04:57:20Z

Thanks for the PR. Could you point us to the convo that you're mentioning? And as you touch upon it, it'd be great if you could provide some benchmarks as well.

danielhanchen · 2019-06-28T05:51:55Z

Oh don't worry now :) It was about some performance issues of SVD. It seems like my changes didn't make SVD faster, just it reduced overall memory usage.
However, if memory reduction is useful, then I can continue on this PR :)

hvy · 2019-07-22T03:35:39Z

cupy/linalg/decomposition.py

+        trans_flag = False
+        X = a.astype(a_dtype, order = 'F', copy = True)
+
+


A detail but let's remove one redundant empty line.

Suggested change

hvy · 2019-07-22T03:35:51Z

cupy/linalg/decomposition.py

+        X = a.astype(a_dtype, order = 'F', copy = True)
+
+
+    MIN = min(n, p)


Upper case variables looks like constants. How about min_mn?

I think k is a quite common convention in the context of SVD.

Also, could you change the other upper-cased variables back to lower case too?

hvy · 2019-07-22T03:36:48Z

cupy/linalg/decomposition.py

-        m, n = a.shape
-        x = a.transpose().astype(a_dtype, order='C', copy=True)
+
+    n, p = a.shape


Why not keep the previous m?

Ye soz it was a quick refactor to test if my changes actually made SVD faster. So to make my life easier I decided to use n, p (statistics convention) rather than the general m, n.

Could you change it back to the previous naming convention?

hvy · 2019-07-22T03:37:57Z

cupy/linalg/decomposition.py

+        n, p = X.shape
+    else:
+        trans_flag = False
+        X = a.astype(a_dtype, order = 'F', copy = True)


A style convention but can you skip the whitespaces when specifying kwargs?

hvy · 2019-07-22T04:47:59Z

cupy/linalg/decomposition.py

+        if a.flags.c_contiguous:
+            X = a.astype(a_dtype, order = 'C', copy = True).transpose()
+        else:
+            X = a.transpose().astype(a_dtype, order = 'F', copy = True)


Could you add tests for this code path?

hvy · 2019-07-22T04:50:30Z

cupy/linalg/decomposition.py

        trans_flag = True
-    mn = min(m, n)
+        # Perform SVD(X.T) not SVD(X) This reduces complexity from
+        # O(np^2) to O(n^2p) if p > n


Would it be possible to provide some benchmarking results (speed and memory)? Also does this comment mean that you didn't see any speed improvements? #2277 (comment)

danielhanchen · 2019-07-23T13:31:00Z

Oh hey @hvy :) Yep so the PR Im placing was primarily for speed improvements. I think @pentschev tested my code, and we found out that the latest version of cuSOLVER was far faster than the current version + it's faster than MKL.

However, I guess the only benefit of my PR is that memory usage is reduced (ie by approx np or mn).
I haven't tested if internally in cuSOLVER memory is reduced though.

Oh loll ye the styling is probs from my C / C++ coding style.......

hvy · 2019-07-26T07:52:31Z

I see. It makes sense to make use of 'O' when it's possible but I think we still want some benchmarks (both mem and speed) before merging this PR, does that sound reasonable? Also, could you take a look at my comments above? :)

hvy · 2019-07-26T08:00:35Z

cupy/linalg/decomposition.py


    if compute_uv:
        if full_matrices:
-            u = cupy.empty((m, m), dtype=a_dtype)
-            vt = cupy.empty((n, n), dtype=a_dtype)
+            U = cupy.empty((n, n), dtype = a_dtype, order = 'F')


Sorry another comment on the style but can you stick to the following?

Suggested change

U = cupy.empty((n, n), dtype = a_dtype, order = 'F')

U = cupy.empty((n, n), dtype=a_dtype, order='F')

hvy · 2019-07-26T08:02:32Z

cupy/linalg/decomposition.py

            workspace.data.ptr, buffersize, 0, dev_info.data.ptr)

+


Let's skip this line.

Suggested change

takagi · 2020-04-16T04:50:12Z

@danielhanchen Do you still have interest in this PR? Otherwise, can I take this over or close?

danielhanchen · 2020-04-16T06:04:12Z

@takagi Hey! Sorry I kind of forget about this PR lol. Possibly it's best if someone else takes over / or closes this. Thanks!

takagi · 2020-04-17T08:24:38Z

Thanks for your response, then let me take it over.

takagi · 2020-05-10T02:08:12Z

Let me close this PR for now and open another issue to reduce cupy.svd memory usage.

takagi · 2020-05-12T04:56:16Z

@danielhanchen Is it ensured that, if you remember, cusolverDn<t>gesvd() can safely take the same memory region for A and U (or VT)? I've read through cuSOLVER reference, but am not sure that. Just not officially documented?

danielhanchen · 2020-05-12T06:27:19Z

@takagi In https://github.com/cupy/cupy/blob/master/cupy/linalg/decomposition.py, both paths m >=n and m < n copy the input matrix A: x = a.astype(a_dtype, order='C', copy=True) and x = a.transpose().astype(a_dtype, order='C', copy=True).

In https://docs.nvidia.com/cuda/cusolver/index.html#cuds-lt-t-gt-gesvd, only if full_matrices=False (which is generally the use case for SVD), jobu = 'S'/'O' and jobvt = 'S'/'O' already overwrites the data on A. (ie the docs mention that). So, to save an extra nn or mm memory space, reuse the already copied A.

takagi · 2020-05-12T08:16:17Z

Thanks, I got it!

danielhanchen added 3 commits June 26, 2019 22:50

Make SVD overwrite temporary array x

9adf710

@pentschev SVD can now auto overwrite the temporary array x. Can improve performance possibly (haven't tested yet).

Change to F Contiguous

7574ebe

Fixed output for VT to VT[:MIN]

0f6a884

niboshi assigned hvy Jun 27, 2019

hvy suggested changes Jun 27, 2019

View reviewed changes

hvy added the cat:performance Performance in terms of speed or memory consumption label Jun 27, 2019

hvy suggested changes Jul 22, 2019

View reviewed changes

hvy reviewed Jul 26, 2019

View reviewed changes

cupy/linalg/decomposition.py

workspace.data.ptr, buffersize, 0, dev_info.data.ptr)

Copy link

Member

hvy Jul 26, 2019

Choose a reason for hiding this comment

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

Let's skip this line.

Suggested change

hvy added the st:awaiting-author Awaiting response from author label Oct 30, 2019

emcastillo added this to the v8.0.0b2 milestone Mar 23, 2020

kmaehashi assigned takagi and unassigned hvy Mar 31, 2020

takagi removed the st:awaiting-author Awaiting response from author label Apr 23, 2020

asi1024 modified the milestones: v8.0.0b2, v8.0.0b3 Apr 23, 2020

takagi mentioned this pull request May 10, 2020

Reduce memory usage of cupy.linalg.svd #3327

Closed

takagi closed this May 10, 2020

takagi mentioned this pull request May 17, 2020

Reduce memory usage of cupy.linalg.svd #3347

Merged

asi1024 modified the milestones: v8.0.0b3, Closed PRs Dec 24, 2020

Make SVD overwrite temporary array x #2277

Make SVD overwrite temporary array x #2277

danielhanchen commented Jun 26, 2019

hvy Jun 27, 2019

hvy commented Jun 27, 2019

danielhanchen commented Jun 28, 2019

hvy Jul 22, 2019

hvy Jul 22, 2019

hvy Jul 26, 2019

hvy Jul 26, 2019

hvy Jul 22, 2019

danielhanchen Jul 23, 2019

hvy Jul 26, 2019

hvy Jul 22, 2019

hvy Jul 22, 2019

hvy Jul 22, 2019

danielhanchen commented Jul 23, 2019

hvy commented Jul 26, 2019

hvy Jul 26, 2019

hvy Jul 26, 2019

takagi commented Apr 16, 2020

danielhanchen commented Apr 16, 2020

takagi commented Apr 17, 2020

takagi commented May 10, 2020

takagi commented May 12, 2020

danielhanchen commented May 12, 2020

takagi commented May 12, 2020

	# Note that the returned array may need to be transprosed
	# Note that the returned array may need to be transposed

		trans_flag = False
		X = a.astype(a_dtype, order = 'F', copy = True)

		X = a.astype(a_dtype, order = 'F', copy = True)


		MIN = min(n, p)

	U = cupy.empty((n, n), dtype = a_dtype, order = 'F')
	U = cupy.empty((n, n), dtype=a_dtype, order='F')

Make SVD overwrite temporary array x #2277

Make SVD overwrite temporary array x #2277

Conversation

danielhanchen commented Jun 26, 2019

Choose a reason for hiding this comment

hvy commented Jun 27, 2019

danielhanchen commented Jun 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhanchen commented Jul 23, 2019

hvy commented Jul 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takagi commented Apr 16, 2020

danielhanchen commented Apr 16, 2020

takagi commented Apr 17, 2020

takagi commented May 10, 2020

takagi commented May 12, 2020

danielhanchen commented May 12, 2020

takagi commented May 12, 2020