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

Reply via email to