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

Fix erroneous input mutation in linalg routines #5879

Merged
merged 10 commits into from Jul 20, 2020

Conversation

stuartarchibald
Copy link
Contributor

@stuartarchibald stuartarchibald commented Jun 17, 2020

This fixes an issue where inputs were not correctly being copied on the way into linear algebra routines that destroy data during computation. The issue stemmed from np.asfortranarray being called on 'A' ordered arrays, and, in the case that they were already 'F' ordered at run time, no copy would be made. The following implements a new helper function to ensure that 'A' ordered arrays have an appropriate copy made at runtime, fixes up the implementations that were incorrect to now use this, and adds tests for functions that would hit these code paths.

Fixes #5870
Fixes #3368
Fixes #5933

@sklam
Copy link
Member

sklam commented Jul 3, 2020

Can you add testing to _copy_to_fortran_order?

for example:

diff --git a/numba/tests/test_linalg.py b/numba/tests/test_linalg.py
index 235ff8ad9..61cc768e3 100644
--- a/numba/tests/test_linalg.py
+++ b/numba/tests/test_linalg.py
@@ -8,7 +8,7 @@ import platform
 
 import numpy as np
 
-from numba import jit
+from numba import jit, njit, typeof
 from numba.core import errors
 from numba.tests.support import (TestCase, tag, needs_lapack, needs_blas,
                                  _is_armv7l, skip_ppc64le_issue4026)
@@ -1577,8 +1577,8 @@ class TestLinalgLstsq(TestLinalgSystems):
         empties = [
         [(0, 1), (1,)], # empty A, valid b
         [(1, 0), (1,)], # empty A, valid b
-        [(1, 1), (0,)], # valid A, empty 1D b 
-        [(1, 1), (1, 0)],  # valid A, empty 2D b 
+        [(1, 1), (0,)], # valid A, empty 1D b
+        [(1, 1), (1, 0)],  # valid A, empty 2D b
         ]
 
         for A, b in empties:
@@ -2646,5 +2646,46 @@ class TestBasics(TestLinalgSystems):  # TestLinalgSystems for 1d test
         self.assert_error(cfunc, args, msg, err=errors.TypingError)
 
 
+class TestHelpers(TestCase):
+    def test_copy_to_fortran_order(self):
+        from numba.np.linalg import _copy_to_fortran_order
+
+        def check(udt, expectfn, shapes, dtypes, orders):
+            for shape, dtype, order in product(shapes, dtypes, orders):
+                a = np.arange(np.prod(shape)).reshape(shape, order=order)
+
+                r = udt(a)
+                # check correct operation
+                self.assertPreciseEqual(expectfn(a), r)
+                # check new copy has made
+                self.assertNotEqual(a.ctypes.data, r.ctypes.data)
+
+        @njit
+        def direct_call(a):
+            return _copy_to_fortran_order(a)
+
+        shapes = [(3, 4), (3, 2, 5)]
+        dtypes = [np.intp]
+        orders = ['C', 'F']
+        check(direct_call, np.asfortranarray, shapes, dtypes, orders)
+
+
+        @njit
+        def slice_to_any(a):
+            # make a 'any' layout slice
+            sliced = a[::2][0]
+            return _copy_to_fortran_order(sliced)
+
+        shapes = [(3, 3, 4), (3, 3, 2, 5)]
+        dtypes = [np.intp]
+        orders = ['C', 'F']
+
+        def expected_slice_to_any(a):
+            # make a 'any' layout slice
+            sliced = a[::2][0]
+            return np.asfortranarray(sliced)
+
+        check(slice_to_any, expected_slice_to_any, shapes, dtypes, orders)
+
 if __name__ == '__main__':
     unittest.main()

@sklam sklam added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Jul 3, 2020
@sklam
Copy link
Member

sklam commented Jul 3, 2020

Everything else looks good.

@stuartarchibald
Copy link
Contributor Author

Thanks for the review, patch suggested here #5879 (comment) is applied as afa20e0.

@stuartarchibald stuartarchibald added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Jul 6, 2020
@sklam sklam added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Jul 20, 2020
Copy link
Member

@sklam sklam left a comment

Choose a reason for hiding this comment

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

Thanks for the patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge
Projects
None yet
2 participants