Baunsgaard commented on code in PR #1986:
URL: https://github.com/apache/systemds/pull/1986#discussion_r1460144152


##########
src/main/java/org/apache/sysds/hops/rewrite/RewriteAlgebraicSimplificationDynamic.java:
##########
@@ -1137,46 +1138,88 @@ else if( right.getDim2()>1 ) //multi column vector
                
                return hi;
        }
-       
-       private static Hop simplifyDiagMatrixMult(Hop parent, Hop hi, int pos)
-       {
-               if( hi instanceof ReorgOp && 
((ReorgOp)hi).getOp()==ReOrgOp.DIAG && hi.getDim2()==1 ) //diagM2V
+
+       private static Hop simplifyDiagMatrixMult(Hop parent, Hop hi, int pos) {
+               if(hi instanceof ReorgOp && ((ReorgOp) hi).getOp() == 
ReOrgOp.DIAG && hi.getDim2() == 1) //diagM2V
                {
                        Hop hi2 = hi.getInput().get(0);
-                       if( HopRewriteUtils.isMatrixMultiply(hi2) ) //X%*%Y
+                       if(HopRewriteUtils.isMatrixMultiply(hi2)) //X%*%Y
                        {
                                Hop left = hi2.getInput().get(0);
                                Hop right = hi2.getInput().get(1);
-                               
+
                                //create new operators (incl refresh size 
inside for transpose)
                                ReorgOp trans = 
HopRewriteUtils.createTranspose(right);
                                BinaryOp mult = 
HopRewriteUtils.createBinary(left, trans, OpOp2.MULT);
                                AggUnaryOp rowSum = 
HopRewriteUtils.createAggUnaryOp(mult, AggOp.SUM, Direction.Row);
-                               
+
                                //rehang new subdag under parent node
                                HopRewriteUtils.replaceChildReference(parent, 
hi, rowSum, pos);
                                HopRewriteUtils.cleanupUnreferenced(hi, hi2);
-                               
+
                                hi = rowSum;
                                LOG.debug("Applied simplifyDiagMatrixMult");
-                       }       
+                       }
                }
-               
+
                return hi;
        }
-       
-       private static Hop simplifySumDiagToTrace(Hop hi)
-       {
-               if( hi instanceof AggUnaryOp ) 
-               {
+
+       private static Hop simplifyMatrixFactorization(Hop parent, Hop hi, int 
pos) {
+               // Check for Hop being Binary
+               if(hi instanceof BinaryOp) {
+                       Hop left = hi.getInput().get(0);

Review Comment:
   That would be okay. But the best way ( in my opinion) would be to add a Java 
doc on the method to specify exactly what it is intended to do. 
   



-- 
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