Baunsgaard commented on code in PR #2103: URL: https://github.com/apache/systemds/pull/2103#discussion_r1749361900
########## src/main/java/org/apache/sysds/runtime/instructions/cp/ReorgCPInstruction.java: ########## @@ -83,6 +85,31 @@ private ReorgCPInstruction(Operator op, CPOperand in, CPOperand out, CPOperand c _col = col; _desc = desc; _ixret = ixret; + _shift = new CPOperand(); + } + + /** + * for opcode roll + * + * @param op + * operator + * @param in + * cp input operand + * @param shift + * ? + * @param out + * cp output operand + * @param opcode + * the opcode + * @param istr + * ? + */ Review Comment: the formatting is off then, here is a correctly formatted one: ```java /** * Apply binary row operation on the right side with one extra row evaluating with zeros. * * @param op The operation to this dictionary * @param v The values to use on the right hand side. * @param colIndexes The column indexes to consider inside v. * @return A new dictionary containing the updated values. */ public IDictionary binOpRightAndAppend(BinaryOperator op, double[] v, IColIndex colIndexes); ``` ########## src/main/java/org/apache/sysds/runtime/matrix/data/LibMatrixReorg.java: ########## @@ -2243,7 +2271,70 @@ private static void reverseSparse(MatrixBlock in, MatrixBlock out) { if( !a.isEmpty(i) ) c.set(m-1-i, a.get(i), true); } - + + private static void rollDense(MatrixBlock in, MatrixBlock out, int shift) { + final int m = in.rlen; + final int n = in.clen; + + //set basic meta data and allocate output + out.sparse = false; + out.nonZeros = in.nonZeros; + out.allocateDenseBlock(false); + + //copy all rows into target positions + if( n == 1 ) { //column vector + double[] a = in.getDenseBlockValues(); + double[] c = out.getDenseBlockValues(); + + // roll matrix with axis=none + shift %= (m != 0 ? m : 1); + + System.arraycopy(a, 0, c, shift, m - shift); + System.arraycopy(a, m - shift, c, 0, shift); + } else { //general matrix case + DenseBlock a = in.getDenseBlock(); + DenseBlock c = out.getDenseBlock(); + + // roll matrix with axis=0 + shift %= (m != 0 ? m : 1); + + for( int i=0; i< m-shift; i++ ) { + System.arraycopy(a.values(i), a.pos(i), c.values(i + shift), c.pos(i + shift), n); + } + + for( int i=m-shift; i<m; i++ ) { + System.arraycopy(a.values(i), a.pos(i), c.values(i + shift -m), c.pos(i + shift -m), n); + } + } + } + + private static void rollSparse(MatrixBlock in, MatrixBlock out, int shift) { + final int m = in.rlen; + + //set basic meta data and allocate output + out.sparse = true; + out.nonZeros = in.nonZeros; + out.allocateSparseRowsBlock(false); + + //copy all rows into target positions + SparseBlock a = in.getSparseBlock(); + SparseBlock c = out.getSparseBlock(); + + // roll matrix with axis=0 + shift %= (m != 0 ? m : 1); + + for( int i=0; i<m-shift; i++ ) { Review Comment: Great, it can be further optimized, by minor tweaks such as: 1. Move the inner parts of the for loop to a separate function to enable JIT (the two loops can call the same function with different shift arguments) 2. Move the shift addition out of the inner inner loop but to a temporary variable ########## docker/testsysds.Dockerfile: ########## @@ -78,6 +78,13 @@ RUN apt-get install -y --no-install-recommends \ && rm -rf installDependencies.R \ && rm -rf /var/lib/apt/lists/* +RUN apt-get install -y --no-install-recommends \ + python3 python3-pip && \ + apt-get clean && \ + python3 -m pip install --upgrade pip && \ + pip3 install numpy scipy && \ + echo "Python3 installed" Review Comment: This is fine to add, however, i would prefer not to. We have an---unrelated to your PR---problem with the testsysds docker file that would require a larger fix to make it build properly. Therefore, it will take some time before we would run the update and therefore be able to enable the test to call python from inside the system. Fundamentally this blocks your PR, because your work relies on Python being installed on the machine you want to test on. ########## .github/workflows/javaTests.yml: ########## @@ -85,6 +85,7 @@ jobs: "**.functions.reorg.**,**.functions.rewrite.**,**.functions.ternary.**", "**.functions.transform.**","**.functions.unique.**", "**.functions.unary.matrix.**,**.functions.linearization.**,**.functions.jmlc.**" + "**.functions.reorg.**,**.functions.rewrite.**,**.functions.ternary.**" Review Comment: this looks like a duplicate of line 85, therefore only double the number of times we run the test. -- 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