Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[SYSTEMDS-3758] Python API Builtin triu, tril, argmin, argmax and casting Scalar <-> Matrix <-> Frame #2113
base: main
Are you sure you want to change the base?
[SYSTEMDS-3758] Python API Builtin triu, tril, argmin, argmax and casting Scalar <-> Matrix <-> Frame #2113
Changes from all commits
bb17eb0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the formatting comes from black, as you recommended
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, currently the cyclic dependency error is avoided since the import happens inside the function, which won't be executed while importing afaik.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the rest of the functions looks good.