Baunsgaard commented on a change in pull request #975:
URL: https://github.com/apache/systemml/pull/975#discussion_r440771496
##########
File path: src/main/python/systemds/matrix/data_gen.py
##########
@@ -29,6 +29,8 @@
from systemds.operator import OperationNode
from systemds.context import SystemDSContext
+from systemds.matrix import Matrix
+import numpy as np
def full(sds_context: SystemDSContext, shape: Tuple[int, int], value:
Union[float, int]) -> OperationNode:
Review comment:
Also if you want to, then maybe change these data_gen functions to
return matrix Objects, not Operation nods.
##########
File path: src/main/python/systemds/matrix/data_gen.py
##########
@@ -19,7 +19,7 @@
#
# -------------------------------------------------------------
-__all__ = ['full', 'seq', 'rand']
+__all__ = ['full', 'seq', 'rand', 'rev', 'order', 't', 'cholesky']
Review comment:
This file contains methods for data generation, not for operations.
I think it would be nice if you could move the `rev` `order` `t` and
`cholesky` to the matrix.py object. thereby removing the need and potential
error associated with calling these functions with an SystemDSContext.
##########
File path: src/main/python/systemds/matrix/data_gen.py
##########
@@ -102,3 +105,70 @@ def rand(sds_context: SystemDSContext, rows: int, cols:
int,
named_input_nodes['seed'] = seed
return OperationNode(sds_context, 'rand', [],
named_input_nodes=named_input_nodes)
+
+
+def rev(sds_context: 'SystemDSContext', X: Matrix) -> 'OperationNode':
+ """ Reverses the rows in a matrix
+
+ :param sds_context: SystemDS context
+ :param X: Input matrix
+ :return: reversed matrix
+ """
+ X._check_matrix_op()
+ return OperationNode(sds_context, 'rev', [X])
+
+
+def order(sds_context: 'SystemDSContext', X: Matrix, by: int = 1, decreasing:
bool = False, index_return: bool = False) -> 'OperationNode':
+ """ Sort by a column of the matrix X in increasing/decreasing order and
returns either the index or data
+
+ :param sds_context: SystemDS context
+ :param X: Input matrix
+ :param by: Column number
+ :param decreasing: If true the matrix will be sorted in decreasing order
+ :param index_return: If true, theindex numbers will be returned
+ :return: sorted matrix
+ """
+ X._check_matrix_op()
+
+ cols = X._np_array.shape[1]
+ if by > cols:
+ raise IndexError("Index {i} is out of bounds for axis 1 with size
{c}".format(i=by, c=cols))
+
+ named_input_nodes = {'target': X, 'by': by, 'decreasing':
str(decreasing).upper(), 'index.return': str(index_return).upper()}
+
+ return OperationNode(sds_context, 'order', [],
named_input_nodes=named_input_nodes)
+
+
+def t(sds_context: 'SystemDSContext', X: Matrix) -> 'OperationNode':
+ """ Transposes the input matrix
+
+ :param sds_context: SystemDS context
+ :param X: Input matrix
+ :return: transposed matrix
+ """
+ X._check_matrix_op()
+ return OperationNode(sds_context, 't', [X])
+
+
+def cholesky(sds_context: 'SystemDSContext', X: Matrix) -> 'OperationNode':
+ """ Computes the Cholesky decomposition of a symmetric, positive definite
matrix
+
+ :param sds_context: SystemDS context
+ :param X: Input matrix
+ :return: Cholesky decomposition
+ """
+ X._check_matrix_op()
+
+ # check square dimension
+ if X._np_array.shape[0] != X._np_array.shape[1]:
+ raise ValueError("Last 2 dimensions of the array must be square")
+
+ # check if mat is positive definite
+ if not np.all(np.linalg.eigvals(X._np_array)>0):
+ raise ValueError("Matrix is not positive definite")
+
+ # check if mat is symmetric
+ if not np.allclose(X._np_array, X._np_array.transpose()):
+ raise ValueError("Matrix is not symmetric")
Review comment:
I like the checks, but the complexity of these operations grow in size
of X. (except for the first check).
This makes the overall performance of the operation slow, maybe we add an
optional flag saying unsafe, that skip these checks?
##########
File path: src/main/python/systemds/matrix/data_gen.py
##########
@@ -102,3 +105,70 @@ def rand(sds_context: SystemDSContext, rows: int, cols:
int,
named_input_nodes['seed'] = seed
return OperationNode(sds_context, 'rand', [],
named_input_nodes=named_input_nodes)
+
+
+def rev(sds_context: 'SystemDSContext', X: Matrix) -> 'OperationNode':
Review comment:
In the Methods, i would appreciate if the syntax is:
` def rev(sds_context: SystemDSContext, X: Matrix) -> OperationNode:`
Since it allows the system to complain earlier about missing imports and
such not resulting in a potential runtime error.
##########
File path: src/main/python/tests/test_matrix_cholesky.py
##########
@@ -0,0 +1,75 @@
+# -------------------------------------------------------------
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+# -------------------------------------------------------------
+
+import unittest
+
+import numpy as np
+from systemds.context import SystemDSContext
+from systemds.matrix.data_gen import cholesky
+from systemds.matrix import Matrix
+
+np.random.seed(7)
+shape = np.random.randint(1, 100)
+A = np.random.rand(shape, shape)
+# set A = MM^T and A is a positive definite matrix
+A = np.matmul(A, A.transpose())
+
+m1 = -np.random.rand(shape, shape)
+m2 = np.asarray([[4, 9], [1, 4]])
+m3 = np.random.rand(shape, shape + 1)
+
+class TestCholesky(unittest.TestCase):
+
+ sds: SystemDSContext = None
+
+ @classmethod
+ def setUpClass(cls):
+ cls.sds = SystemDSContext()
+
+ @classmethod
+ def tearDownClass(cls):
+ cls.sds.close()
+
+ def test_basic1(self):
+ L = cholesky(self.sds, Matrix(self.sds, A)).compute()
+ self.assertTrue(np.allclose(L, np.linalg.cholesky(A)))
+
+ def test_basic2(self):
+ L = cholesky(self.sds, Matrix(self.sds, A)).compute()
+ # L * L.H = A
+ self.assertTrue(np.allclose(A, np.dot(L, L.T.conj())))
+
+ def test_pos_def(self):
+ with self.assertRaises(ValueError) as context:
+ cholesky(self.sds, Matrix(self.sds, m1)).compute()
+
+ def test_symmetric_matrix(self):
+ np.linalg.cholesky(m2)
+ with self.assertRaises(ValueError) as context:
+ cholesky(self.sds, Matrix(self.sds, m2)).compute()
+
+ def test_asymetric_dim(self):
+ with self.assertRaises(ValueError) as context:
+ cholesky(self.sds, Matrix(self.sds, m3)).compute()
+
+
+if __name__ == "__main__":
+ unittest.main(exit=False)
Review comment:
If you want to make git happy, then add a single newline in the end of
the files.
##########
File path: src/main/python/tests/test_matrix_reverse.py
##########
@@ -0,0 +1,68 @@
+# -------------------------------------------------------------
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+# -------------------------------------------------------------
+import math
+import os
+import random
+import sys
+import unittest
+
+import numpy as np
+from systemds.context import SystemDSContext
+from systemds.matrix.data_gen import rev
+from systemds.matrix import Matrix
+
+np.random.seed(7)
+
+shape = (random.randrange(1, 25), random.randrange(1, 25))
+m = np.random.rand(shape[0], shape[1])
+mx = np.random.rand(1, shape[1])
+my = np.random.rand(shape[0], 1)
+
+class TestReverse(unittest.TestCase):
+
+ sds: SystemDSContext = None
+
+ @classmethod
+ def setUpClass(cls):
+ cls.sds = SystemDSContext()
+
+ @classmethod
+ def tearDownClass(cls):
+ cls.sds.close()
+
+ def test_basic(self):
+ r = rev(self.sds, Matrix(self.sds, m)).compute()
Review comment:
Just a single example of the syntax i think would be great:
`r = Matrix(self.sds, m).rev().compute()`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
