You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@systemds.apache.org by GitBox <gi...@apache.org> on 2021/10/05 12:55:08 UTC
[GitHub] [systemds] Baunsgaard commented on a change in pull request #1406: [SYSTEMDS-2836] Extended Update In-Place Framework
Baunsgaard commented on a change in pull request #1406:
URL: https://github.com/apache/systemds/pull/1406#discussion_r722201181
##########
File path: src/main/java/org/apache/sysds/lops/Unary.java
##########
@@ -122,7 +122,12 @@ private String getOpcode() {
}
public static boolean isMultiThreadedOp(OpOp1 op) {
- return op==OpOp1.CUMSUM
+ if(op == OpOp1.INVERSE || op == OpOp1.CHOLESKY || op == OpOp1.DETECTSCHEMA)
Review comment:
What is the reason to change which ops are multi-threaded?
##########
File path: src/main/java/org/apache/sysds/runtime/matrix/data/MatrixBlock.java
##########
@@ -2764,7 +2780,15 @@ else if(!sparse && !isEmptyBlock(false)
//note: we apply multi-threading in a best-effort manner here
//only for expensive operators such as exp, log, sigmoid, because
//otherwise allocation, read and write anyway dominates
- ret.allocateDenseBlock(false);
+ if (!op.isInplace() || this.isEmpty())
+ { //allocate dense output block
+ ret.allocateDenseBlock(false);
+ }
+ else
+ {
+ ret = this;
+ }
+ //ret.allocateDenseBlock(false);
Review comment:
formatting.
- { should be same line as statement
- if there is only one line in the if or else statement then remove "{" "}" (the case here)
- Remove commented code that is not used.
##########
File path: src/main/java/org/apache/sysds/runtime/matrix/data/LibMatrixAgg.java
##########
@@ -336,7 +338,7 @@ public static MatrixBlock cumaggregateUnaryMatrix(MatrixBlock in, MatrixBlock ou
AggregateUnaryOperator uaop = InstructionUtils.parseBasicCumulativeAggregateUnaryOperator(uop);
//fall back to sequential if necessary or agg not supported
- if( k <= 1 || (long)in.rlen*in.clen < PAR_NUMCELL_THRESHOLD1 || in.rlen <= k
+ if( k <= 1 || (long)in.rlen*in.clen < PAR_NUMCELL_THRESHOLD1 || in.rlen <= k
Review comment:
fix formatting / I would suggest that you do not make any changes in this file.
##########
File path: src/test/scripts/functions/builtin/updateInPlaceTest.dml
##########
@@ -0,0 +1,16 @@
+#A = rand(rows = 100, cols = 100)
+#C = rand(rows = 100, cols = 100)
+
+A = matrix(0, 10, 10);
+C = matrix(0, 10, 10);
+
+for (x in 1 : 10) {
+ A[x,] = matrix(x, rows = 1, cols = 10);
+ C[x,] = matrix(x, rows = 1, cols = 10);
+}
+B = round(A)
+D = log(C)
+C = B+D * 3
+Out = C
+write(Out, $Out);
+print(as.scalar(C[2, 1]))
Review comment:
new line in the end of the file.
##########
File path: src/main/java/org/apache/sysds/runtime/matrix/data/MatrixBlock.java
##########
@@ -2730,7 +2730,23 @@ public MatrixBlock scalarOperations(ScalarOperator op, MatrixValue result) {
return ret;
}
-
+ //ToDo
+ /*
+ extend the runtime operations in MatrixBlock
+ unaryOperations(UnaryOperator op, MatrixValue result),
+ which are called from UnaryMatrixCPInstruction,
+ so they exploit the update-in-place flag
+ by directly overwriting the input
+ (this can be done by simply assigning the input instead of allocating a new block).
+ So far, we only support this in the branch for cumaggregateUnaryMatrix,
+ but you should do that for multi-threaded and denseUnary/sparseUnary operations
+ on dense inputs&outputs (if both are dense) too
+ (later we'll extend that to sparse formats).
+ All extensions would be local to unaryOperations and potentially denseUnaryOperations
+ and sparseUnaryOperations.
+ Have a look into the cumaggregateUnaryMatrix to see how to use the flag from the passed
+ operator object.
+ */
Review comment:
Please remove this comment, instead do what you suggest, and throw a NotImplementedException in cases that are not implemented yet.
##########
File path: src/main/java/org/apache/sysds/runtime/instructions/cp/UnaryCPInstruction.java
##########
@@ -61,8 +63,9 @@ public static UnaryCPInstruction parseInstruction ( String str ) {
in.split(parts[1]);
out.split(parts[2]);
func = Builtin.getBuiltinFnObject(opcode);
-
- if( Arrays.asList(new String[]{"ucumk+","ucum*","ucumk+*","ucummin","ucummax","exp","log","sigmoid"}).contains(opcode) ){
+ Types.OpOp1 op_type = Types.OpOp1.valueOfByOpcode(opcode);
+ //if( Arrays.asList(new String[]{"ucumk+","ucum*","ucumk+*","ucummin","ucummax","exp","log","sigmoid","round"}).contains(opcode) )
+ if( Unary.isMultiThreadedOp(op_type)){
Review comment:
This change here is twofold.
1. yes it is great that you changed it to not compare a list of strings,
2. it seems like it allows fewer operations to enter the loop.
##########
File path: src/main/java/org/apache/sysds/runtime/matrix/data/MatrixBlock.java
##########
@@ -2846,8 +2871,14 @@ else if( sparse ) //DENSE <- SPARSE
}
else //DENSE <- DENSE
{
- //allocate dense output block
- ret.allocateDenseBlock(false);
+ if (!op.isInplace() || this.isInSparseFormat() || this.isEmpty())
+ { //allocate dense output block
+ ret.allocateDenseBlock(false);
+ }
+ else
+ {
+ ret = this;
+ }
Review comment:
formatting
##########
File path: src/test/java/org/apache/sysds/test/functions/builtin/BuiltInInPlaceTest.java
##########
@@ -0,0 +1,63 @@
+package org.apache.sysds.test.functions.builtin;
Review comment:
add license
##########
File path: src/test/java/org/apache/sysds/test/functions/builtin/BuiltInInPlaceTest.java
##########
@@ -0,0 +1,63 @@
+package org.apache.sysds.test.functions.builtin;
+
+import org.apache.sysds.common.Types;
+import org.apache.sysds.hops.OptimizerUtils;
+import org.apache.sysds.runtime.matrix.data.MatrixValue;
+import org.apache.sysds.test.AutomatedTestBase;
+import org.apache.sysds.test.TestConfiguration;
+import org.apache.sysds.test.TestUtils;
+import org.junit.Test;
+
+import java.util.HashMap;
+
+
+public class BuiltInInPlaceTest extends AutomatedTestBase{
+ private final static String TEST_NAME = "updateInPlaceTest";
+ private final static String TEST_DIR = "functions/builtin/";
+ private final static String TEST_CLASS_DIR = TEST_DIR + BuiltinSplitTest.class.getSimpleName() + "/";
+ private final static double eps = 1e-3;
+
+ @Override
+ public void setUp() {
+ TestUtils.clearAssertionInformation();
+ addTestConfiguration(TEST_NAME, new TestConfiguration(TEST_CLASS_DIR, TEST_NAME, new String[]{"B",}));
+ }
+
+ @Test
+ public void testInPlace() {
+ runInPlaceTest(Types.ExecType.CP);
+ }
+
+
+ private void runInPlaceTest(Types.ExecType instType) {
+ Types.ExecMode platformOld = setExecMode(instType);
+ try {
+ loadTestConfiguration(getTestConfiguration(TEST_NAME));
+ String HOME = SCRIPT_DIR + TEST_DIR;
+ fullDMLScriptName = HOME + TEST_NAME + ".dml";
+ programArgs = new String[]{"-nvargs","Out=" + output("Out") };
+
+ //double[][] A = getRandomMatrix(size, 1, -10, 10, 0.6, 7);
+ //writeInputMatrixWithMTD("A", A, true);
+ OptimizerUtils.ENABLE_UNARY_UPDATE_IN_PLACE = true;
+ runTest(true, false, null, -1);
+ HashMap<MatrixValue.CellIndex, Double> dmlfileOut1 = readDMLMatrixFromOutputDir("Out");
+ OptimizerUtils.ENABLE_UNARY_UPDATE_IN_PLACE = false;
+ runTest(true, false, null, -1);
+ HashMap<MatrixValue.CellIndex, Double> dmlfileOut2 = readDMLMatrixFromOutputDir("Out");
+
+ //compare matrices
+ // HashMap<MatrixValue.CellIndex, Double> dmlfileOut1 = readDMLMatrixFromOutputDir("Out");
+ // HashMap<MatrixValue.CellIndex, Double> rfileOut1 = readRMatrixFromExpectedDir("Out");
+ // TestUtils.compareMatrices(dmlfileOut1, rfileOut1, eps, "Stat-DML", "Stat-R");
+ // TestUtils.compareScalars(1,1,eps);
+ TestUtils.compareMatrices(dmlfileOut1,dmlfileOut2,eps,"Stat-DML1","Stat-DML2");
+ }
+ catch(Exception e) {
+ e.printStackTrace();
+ }
+ finally {
+ rtplatform = platformOld;
+ }
+ }
+}
Review comment:
add newline in the end of the file
##########
File path: src/main/java/org/apache/sysds/runtime/matrix/data/MatrixBlock.java
##########
@@ -2773,7 +2797,8 @@ else if(!sparse && !isEmptyBlock(false)
}
ret.recomputeNonZeros();
}
- else {
+ else
+ {
Review comment:
formatting
##########
File path: src/main/java/org/apache/sysds/hops/UnaryOp.java
##########
@@ -165,10 +167,25 @@ else if(_op == OpOp1.MEDIAN) {
}
else //default unary
{
+ boolean inplace = false;
+
+ if (OptimizerUtils.ENABLE_UNARY_UPDATE_IN_PLACE) {
Review comment:
Great check, please make it a method that can be called that return a boolean, and put that method in src/main/java/org/apache/sysds/hops/Hop.java.
call it something like:
canBeUpdateInPlace()
##########
File path: src/test/scripts/functions/builtin/updateInPlaceTest.dml
##########
@@ -0,0 +1,16 @@
+#A = rand(rows = 100, cols = 100)
Review comment:
add license
--
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-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org