Baunsgaard commented on code in PR #2113:
URL: https://github.com/apache/systemds/pull/2113#discussion_r1772073890
##########
src/main/python/systemds/operator/nodes/matrix.py:
##########
@@ -634,6 +634,14 @@ def to_string(self, **kwargs: Dict[str,
VALID_INPUT_TYPES]) -> "Scalar":
self.sds_context, "toString", [self], kwargs,
output_type=OutputType.STRING
)
+ def to_scalar(self):
+ return Scalar(self.sds_context, "as.scalar", [self])
+
+ def to_frame(self):
+ from systemds.operator import Frame
Review Comment:
cyclic dependency, i understand it is how i thought about it as well,
however, maybe we need to add it afterwards dynamically instead.
See:
https://stackoverflow.com/questions/972/adding-a-method-to-an-existing-object-instance-in-python
This would allow us to add the to_scalar, to_frame and to_matrix on the
operation_node directly inside frame, matrix and scalar themselves to overwrite
the operation node.
This would also allow us to have documentation on the method, since you add
an empty method with docs that would throw an exception on the operation_node,
and then on the import of Matrix etc it overwrite the Class method such that
new instances use the overwritten method in Matrix.
##########
src/main/python/systemds/operator/nodes/frame.py:
##########
Review Comment:
most of the changes in this file are formatting, i would prefer if we add
the formatting in a different commit.
##########
src/main/python/systemds/script_building/dag.py:
##########
@@ -43,6 +43,8 @@ class OutputType(Enum):
NONE = auto()
SCALAR = auto()
STRING = auto()
+ INT = auto()
+ BOOLEAN = auto()
Review Comment:
These are not valid, since both are instances of SCALAR,
where are they used?
##########
src/main/python/systemds/script_building/dag.py:
##########
Review Comment:
same here, move the formatting to another PR.
##########
src/main/python/systemds/operator/nodes/matrix.py:
##########
@@ -810,5 +818,89 @@ def quantile(self, p, weights: "Matrix" = None) ->
"OperationNode":
else:
raise ValueError("P has to be a Scalar or Matrix")
+ def triu(self, include_diagonal=True, return_values=True) -> "Matrix":
+ """Selects the upper triangular part of a matrix, configurable to
include the diagonal and return values or ones
+
+ :param include_diagonal: boolean, default True
+ :param return_values: boolean, default True, if set to False returns
ones
+ :return: `Matrix`
+ """
+ named_input_nodes = {
+ "target": self,
+ "diag": self.sds_context.scalar(include_diagonal),
+ "values": self.sds_context.scalar(return_values),
+ }
+ return Matrix(
+ self.sds_context, "upper.tri", named_input_nodes=named_input_nodes
+ )
+
+ def tril(self, include_diagonal=True, return_values=True) -> "Matrix":
+ """Selects the lower triangular part of a matrix, configurable to
include the diagonal and return values or ones
+
+ :param include_diagonal: boolean, default True
+ :param return_values: boolean, default True, if set to False returns
ones
+ :return: `Matrix`
+ """
+ named_input_nodes = {
+ "target": self,
+ "diag": self.sds_context.scalar(include_diagonal),
+ "values": self.sds_context.scalar(return_values),
+ }
+ return Matrix(
+ self.sds_context, "lower.tri", named_input_nodes=named_input_nodes
+ )
+
+ def argmin(self, axis: int = None) -> "OperationNode":
+ """Return the index of the minimum if axis is None or a column vector
for row-wise / column-wise minima
+ computation.
+
+ :param axis: can be 0 or 1 to do either row or column sums
+ :return: `Matrix` representing operation for row / columns or 'Scalar'
representing operation for complete
+ """
+ if axis == 0:
+ return Matrix(self.sds_context, "rowIndexMin", [self.t()])
+ elif axis == 1:
+ return Matrix(self.sds_context, "rowIndexMin", [self])
+ elif axis is None:
+ return Matrix(
+ self.sds_context,
+ "rowIndexMin",
+ [self.reshape(1, self.nCol() * self.nRow())],
+ ).to_scalar()
+ else:
+ raise ValueError(
+ f"Axis has to be either 0, 1 or None, for column, row or
complete {self.operation}"
+ )
+
+ def argmax(self, axis: int = None) -> "OperationNode":
+ """Return the index of the maximum if axis is None or a column vector
for row-wise / column-wise maxima
+ computation.
+
+ :param axis: can be 0 or 1 to do either row or column sums
+ :return: `Matrix` representing operation for row / columns or 'Scalar'
representing operation for complete
+ """
+ if axis == 0:
+ return Matrix(self.sds_context, "rowIndexMax", [self.t()])
+ elif axis == 1:
+ return Matrix(self.sds_context, "rowIndexMax", [self])
+ elif axis is None:
+ return Matrix(
+ self.sds_context,
+ "rowIndexMax",
+ [self.reshape(1, self.nCol() * self.nRow())],
+ ).to_scalar()
+ else:
+ raise ValueError(
+ f"Axis has to be either 0, 1 or None, for column, row or
complete {self.operation}"
+ )
+
+ def reshape(self, rows, cols=1):
+ """Gives a new shape to a matrix without changing its data.
+
+ :param rows: number of rows
+ :param cols: number of columns, defaults to 1
+ :return: `Matrix` representing operation"""
+ return Matrix(self.sds_context, "matrix", [self, rows, cols])
Review Comment:
the rest of the functions looks good.
##########
src/main/python/systemds/operator/nodes/frame.py:
##########
@@ -112,68 +136,94 @@ def transform_apply(self, spec: "Scalar", meta: "Frame"):
params_dict = {"target": self, "spec": spec, "meta": meta}
return Matrix(self.sds_context, "transformapply",
named_input_nodes=params_dict)
- def rbind(self, other) -> 'Frame':
+ def rbind(self, other) -> "Frame":
"""
- Row-wise frame concatenation, by concatenating the second frame as
additional rows to the first frame.
+ Row-wise frame concatenation, by concatenating the second frame as
additional rows to the first frame.
:param: The other frame to bind to the right hand side
:return: The OperationNode containing the concatenated frames.
"""
return Frame(self.sds_context, "rbind", [self, other])
- def cbind(self, other) -> 'Frame':
+ def cbind(self, other) -> "Frame":
"""
- Column-wise frame concatenation, by concatenating the second frame as
additional columns to the first frame.
+ Column-wise frame concatenation, by concatenating the second frame as
additional columns to the first frame.
:param: The other frame to bind to the right hand side.
:return: The Frame containing the concatenated frames.
"""
return Frame(self.sds_context, "cbind", [self, other])
- def replace(self, pattern: str, replacement: str) -> 'Frame':
+ def replace(self, pattern: str, replacement: str) -> "Frame":
"""
Replace all instances of string with replacement string
:param: pattern the string to replace
:param: replacement the string to replace with
- :return: The Frame containing the replaced values
+ :return: The Frame containing the replaced values
"""
- return Frame(self.sds_context, "replace", named_input_nodes={"target":
self, "pattern": f"'{pattern}'", "replacement": f"'{replacement}'"})
+ return Frame(
+ self.sds_context,
+ "replace",
+ named_input_nodes={
+ "target": self,
+ "pattern": f"'{pattern}'",
+ "replacement": f"'{replacement}'",
+ },
+ )
- def to_string(self, **kwargs: Dict[str, VALID_INPUT_TYPES]) -> 'Scalar':
- """ Converts the input to a string representation.
+ def to_string(self, **kwargs: Dict[str, VALID_INPUT_TYPES]) -> "Scalar":
+ """Converts the input to a string representation.
:return: `Scalar` containing the string.
"""
- return Scalar(self.sds_context, 'toString', [self], kwargs,
output_type=OutputType.STRING)
+ return Scalar(
+ self.sds_context, "toString", [self], kwargs,
output_type=OutputType.STRING
+ )
+
+ def to_matrix(self):
+ return Matrix(self.sds_context, "as.matrix", [self])
+
+ def to_scalar(self):
+ return Scalar(self.sds_context, "as.scalar", [self])
def __str__(self):
return "FrameNode"
- def nRow(self) -> 'Scalar':
- return Scalar(self.sds_context, 'nrow', [self])
+ def nRow(self) -> "Scalar":
Review Comment:
i am fine with changing all of the return types to use " instead of ', but
does it really make a difference?
##########
src/main/python/systemds/operator/nodes/scalar.py:
##########
Review Comment:
please move the formatting to another PR
--
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.
To unsubscribe, e-mail: dev-unsubscr...@systemds.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
