[SYSTEMDS-3758] Python API Builtin triu, tril, argmin, argmax and casting Scalar <-> Matrix <-> Frame #2113
Conversation
…ting Scalar <-> Matrix <-> Frame
|
|
||
| def __str__(self): | ||
| return "FrameNode" | ||
|
|
||
| def nRow(self) -> 'Scalar': | ||
| return Scalar(self.sds_context, 'nrow', [self]) | ||
| def nRow(self) -> "Scalar": |
There was a problem hiding this comment.
i am fine with changing all of the return types to use " instead of ', but does it really make a difference?
There was a problem hiding this comment.
All the formatting comes from black, as you recommended
There was a problem hiding this comment.
okay, maybe we should add a verification in Github actions that analyze if the code base is all formated according to black, i have seen this in other repositories such as Modyn
The check would be simple to add in a new action using:
https://black.readthedocs.io/en/stable/usage_and_configuration/the_basics.html#check
| return Scalar(self.sds_context, "as.scalar", [self]) | ||
|
|
||
| def to_frame(self): | ||
| from systemds.operator import Frame |
There was a problem hiding this 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.
There was a problem hiding this comment.
I see, currently the cyclic dependency error is avoided since the import happens inside the function, which won't be executed while importing afaik.
| :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]) |
There was a problem hiding this comment.
the rest of the functions looks good.
There was a problem hiding this comment.
please move the formatting to another PR
There was a problem hiding this comment.
same here, move the formatting to another PR.
| @@ -43,6 +43,8 @@ class OutputType(Enum): | |||
| NONE = auto() | |||
| SCALAR = auto() | |||
| STRING = auto() | |||
| INT = auto() | |||
| BOOLEAN = auto() | |||
There was a problem hiding this comment.
These are not valid, since both are instances of SCALAR,
where are they used?
There was a problem hiding this comment.
But isnt STRING also a value type of Scalar?
I added boolean and int there since in the scalar class, the value type of the scalar is determined by this. But you're right it would make more sense to put the value type in a different enum and create a new attribute, so that there is one field for the data type (matrix, frame, scalar) and one for the value types (double, integer, string, boolean)
There was a problem hiding this comment.
you are right, that would make more sense.
I think the reason why it is as it is now, is because someone (probably me) was lazy when making the initial version.
This patch adds the builtin functions tril, triu, argmin (rowIndexMin) and argmax (rowIndexMax) to the python API. Additionally, I added functionalities to the python API to
I also extended the Scalar Output Type with boolean and integer