Skip to content

Printing matrix without toString()#2180

Closed
ReneEnjilian wants to merge 5 commits intoapache:mainfrom
ReneEnjilian:matrix-printing
Closed

Printing matrix without toString()#2180
ReneEnjilian wants to merge 5 commits intoapache:mainfrom
ReneEnjilian:matrix-printing

Conversation

@ReneEnjilian
Copy link
Contributor

This patch enables the DML to print matrices and other datatypes (frame, list) without using the toString() method.
Previously, printing a matrix A required: print(toString(A)).
Now, printing can be done via: print(A).
The same applies to frames and lists. However, there are some remaining issues when datastructures are used with indexing. For example, while A[1,] is also a matrix (first row of A), print(A[1,]) will not print the correct output (first row) but rather only the first element of the first row. Internally, the compiler misinterprets the matrix as a scalar and does not propagate the correct size information. The propagated size information on the rewrite-level are (-1 x -1), meaning unknown. A work-around would be either to still use the toString method or, as shown in the test cases, to materialize the slicing before printing: B=A[1,] , print(B).
Similar problems exist with lists and frames but can be addressed by materializing the slicing/indexing outside the print as shown in the previous example. A future task should address this issue.
I promoted the rewrite from RewriteAlgebraicSimplificationStatic to having a separate class in the rewrite directory as discussed. Testing has been added.

@ReneEnjilian
Copy link
Contributor Author

All checks fail. The error message for all of the 44 failing checks is something like:
Error: fatal: unable to access 'https://github.com/apache/systemds/': The requested URL returned error: 502
Can I assume that this issue is unrelated to my pushed code ?

@mboehm7
Copy link
Contributor

mboehm7 commented Jan 15, 2025

Yes, that seems to be an external issue, but the missing license header (failed license check) is real, and once you made this change we get another run of the tests.

@mboehm7
Copy link
Contributor

mboehm7 commented Jan 18, 2025

LGTM - Thanks for the patch @ReneEnjilian. During the merge, I moved the rewrite to static rewrites (in program rewriter), fixed the failing test, and removed commented code.

@mboehm7 mboehm7 closed this in 9484f11 Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants