[SYSTEMDS-2745] Fix indexed addition assignment (accumulation)
This patch adds the missing support for addition assignments in left
indexing expressions for both scalars and matrices as well as scalar and
matrix indexed ranges.
Thanks to Rene Haubitzer for catching this issue.
diff --git a/src/main/java/org/apache/sysds/parser/DMLTranslator.java b/src/main/java/org/apache/sysds/parser/DMLTranslator.java
index ff41df6..aab0d22 100644
--- a/src/main/java/org/apache/sysds/parser/DMLTranslator.java
+++ b/src/main/java/org/apache/sysds/parser/DMLTranslator.java
@@ -1137,11 +1137,8 @@
if (!(target instanceof IndexedIdentifier)) {
//process right hand side and accumulation
Hop ae = processExpression(source, target, ids);
- if( ((AssignmentStatement)current).isAccumulator() ) {
- DataIdentifier accum = liveIn.getVariable(target.getName());
- if( accum == null )
- throw new LanguageException("Invalid accumulator assignment "
- + "to non-existing variable "+target.getName()+".");
+ if( as.isAccumulator() ) {
+ DataIdentifier accum = getAccumulatorData(liveIn, target.getName());
ae = HopRewriteUtils.createBinary(ids.get(target.getName()), ae, OpOp2.PLUS);
target.setProperties(accum.getOutput());
}
@@ -1170,6 +1167,15 @@
else {
Hop ae = processLeftIndexedExpression(source, (IndexedIdentifier)target, ids);
+ if( as.isAccumulator() ) {
+ DataIdentifier accum = getAccumulatorData(liveIn, target.getName());
+ Hop rix = processIndexingExpression((IndexedIdentifier)target, null, ids);
+ Hop rhs = processExpression(source, null, ids);
+ Hop binary = HopRewriteUtils.createBinary(rix, rhs, OpOp2.PLUS);
+ HopRewriteUtils.replaceChildReference(ae, ae.getInput(1), binary);
+ target.setProperties(accum.getOutput());
+ }
+
ids.put(target.getName(), ae);
// obtain origDim values BEFORE they are potentially updated during setProperties call
@@ -1298,7 +1304,14 @@
}
sb.updateLiveVariablesOut(updatedLiveOut);
sb.setHops(output);
-
+ }
+
+ private static DataIdentifier getAccumulatorData(VariableSet liveIn, String varname) {
+ DataIdentifier accum = liveIn.getVariable(varname);
+ if( accum == null )
+ throw new LanguageException("Invalid accumulator assignment "
+ + "to non-existing variable "+varname+".");
+ return accum;
}
private void appendDefaultArguments(FunctionStatement fstmt, List<String> inputNames, List<Hop> inputs, HashMap<String, Hop> ids) {
@@ -1630,41 +1643,9 @@
return processExpression(source, tmpOut, hops );
}
- private Hop processLeftIndexedExpression(Expression source, IndexedIdentifier target, HashMap<String, Hop> hops)
- {
+ private Hop processLeftIndexedExpression(Expression source, IndexedIdentifier target, HashMap<String, Hop> hops) {
// process target indexed expressions
- Hop rowLowerHops = null, rowUpperHops = null, colLowerHops = null, colUpperHops = null;
-
- if (target.getRowLowerBound() != null)
- rowLowerHops = processExpression(target.getRowLowerBound(),null,hops);
- else
- rowLowerHops = new LiteralOp(1);
-
- if (target.getRowUpperBound() != null)
- rowUpperHops = processExpression(target.getRowUpperBound(),null,hops);
- else
- {
- if ( target.getDim1() != -1 )
- rowUpperHops = new LiteralOp(target.getOrigDim1());
- else {
- rowUpperHops = new UnaryOp(target.getName(), DataType.SCALAR, ValueType.INT64, OpOp1.NROW, hops.get(target.getName()));
- rowUpperHops.setParseInfo(target);
- }
- }
- if (target.getColLowerBound() != null)
- colLowerHops = processExpression(target.getColLowerBound(),null,hops);
- else
- colLowerHops = new LiteralOp(1);
-
- if (target.getColUpperBound() != null)
- colUpperHops = processExpression(target.getColUpperBound(),null,hops);
- else
- {
- if ( target.getDim2() != -1 )
- colUpperHops = new LiteralOp(target.getOrigDim2());
- else
- colUpperHops = new UnaryOp(target.getName(), DataType.SCALAR, ValueType.INT64, OpOp1.NCOL, hops.get(target.getName()));
- }
+ Hop[] ixRange = getIndexingBounds(target, hops, true);
// process the source expression to get source Hops
Hop sourceOp = processExpression(source, target, hops);
@@ -1678,12 +1659,11 @@
if( sourceOp.getDataType().isMatrix() && source.getOutput().getDataType().isScalar() )
sourceOp.setDataType(DataType.SCALAR);
- Hop leftIndexOp = new LeftIndexingOp(target.getName(), target.getDataType(), ValueType.FP64,
- targetOp, sourceOp, rowLowerHops, rowUpperHops, colLowerHops, colUpperHops,
- target.getRowLowerEqualsUpper(), target.getColLowerEqualsUpper());
+ Hop leftIndexOp = new LeftIndexingOp(target.getName(), target.getDataType(),
+ ValueType.FP64, targetOp, sourceOp, ixRange[0], ixRange[1], ixRange[2], ixRange[3],
+ target.getRowLowerEqualsUpper(), target.getColLowerEqualsUpper());
setIdentifierParams(leftIndexOp, target);
-
leftIndexOp.setParseInfo(target);
leftIndexOp.setDim1(target.getOrigDim1());
leftIndexOp.setDim2(target.getOrigDim2());
@@ -1694,38 +1674,7 @@
private Hop processIndexingExpression(IndexedIdentifier source, DataIdentifier target, HashMap<String, Hop> hops) {
// process Hops for indexes (for source)
- Hop rowLowerHops = null, rowUpperHops = null, colLowerHops = null, colUpperHops = null;
-
- if (source.getRowLowerBound() != null)
- rowLowerHops = processExpression(source.getRowLowerBound(),null,hops);
- else
- rowLowerHops = new LiteralOp(1);
-
- if (source.getRowUpperBound() != null)
- rowUpperHops = processExpression(source.getRowUpperBound(),null,hops);
- else
- {
- if ( source.getOrigDim1() != -1 )
- rowUpperHops = new LiteralOp(source.getOrigDim1());
- else {
- rowUpperHops = new UnaryOp(source.getName(), DataType.SCALAR, ValueType.INT64, OpOp1.NROW, hops.get(source.getName()));
- rowUpperHops.setParseInfo(source);
- }
- }
- if (source.getColLowerBound() != null)
- colLowerHops = processExpression(source.getColLowerBound(),null,hops);
- else
- colLowerHops = new LiteralOp(1);
-
- if (source.getColUpperBound() != null)
- colUpperHops = processExpression(source.getColUpperBound(),null,hops);
- else
- {
- if ( source.getOrigDim2() != -1 )
- colUpperHops = new LiteralOp(source.getOrigDim2());
- else
- colUpperHops = new UnaryOp(source.getName(), DataType.SCALAR, ValueType.INT64, OpOp1.NCOL, hops.get(source.getName()));
- }
+ Hop[] ixRange = getIndexingBounds(source, hops, false);
if (target == null) {
target = createTarget(source);
@@ -1735,8 +1684,8 @@
target.setNnz(-1);
Hop indexOp = new IndexingOp(target.getName(), target.getDataType(), target.getValueType(),
- hops.get(source.getName()), rowLowerHops, rowUpperHops, colLowerHops, colUpperHops,
- source.getRowLowerEqualsUpper(), source.getColLowerEqualsUpper());
+ hops.get(source.getName()), ixRange[0], ixRange[1], ixRange[2], ixRange[3],
+ source.getRowLowerEqualsUpper(), source.getColLowerEqualsUpper());
indexOp.setParseInfo(target);
setIdentifierParams(indexOp, target);
@@ -1744,6 +1693,34 @@
return indexOp;
}
+ private Hop[] getIndexingBounds(IndexedIdentifier ix, HashMap<String, Hop> hops, boolean lix) {
+ Hop rowLowerHops = (ix.getRowLowerBound() != null) ?
+ processExpression(ix.getRowLowerBound(),null, hops) : new LiteralOp(1);
+ Hop colLowerHops = (ix.getColLowerBound() != null) ?
+ processExpression(ix.getColLowerBound(),null, hops) : new LiteralOp(1);
+
+ Hop rowUpperHops = null, colUpperHops = null;
+ if (ix.getRowUpperBound() != null)
+ rowUpperHops = processExpression(ix.getRowUpperBound(),null,hops);
+ else {
+ rowUpperHops = ((lix ? ix.getDim1() : ix.getOrigDim1()) != -1) ?
+ new LiteralOp(ix.getOrigDim1()) :
+ new UnaryOp(ix.getName(), DataType.SCALAR, ValueType.INT64, OpOp1.NROW, hops.get(ix.getName()));
+ rowUpperHops.setParseInfo(ix);
+ }
+
+ if (ix.getColUpperBound() != null)
+ colUpperHops = processExpression(ix.getColUpperBound(),null,hops);
+ else {
+ colUpperHops = ((lix ? ix.getDim2() : ix.getOrigDim2()) != -1) ?
+ new LiteralOp(ix.getOrigDim2()) :
+ new UnaryOp(ix.getName(), DataType.SCALAR, ValueType.INT64, OpOp1.NCOL, hops.get(ix.getName()));
+ colUpperHops.setParseInfo(ix);
+ }
+
+ return new Hop[] {rowLowerHops, rowUpperHops, colLowerHops, colUpperHops};
+ }
+
/**
* Construct Hops from parse tree : Process Binary Expression in an
diff --git a/src/test/java/org/apache/sysds/test/functions/indexing/IndexedAdditionAssignmentTest.java b/src/test/java/org/apache/sysds/test/functions/indexing/IndexedAdditionAssignmentTest.java
new file mode 100644
index 0000000..3db2535
--- /dev/null
+++ b/src/test/java/org/apache/sysds/test/functions/indexing/IndexedAdditionAssignmentTest.java
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sysds.test.functions.indexing;
+
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.sysds.common.Types.ExecMode;
+import org.apache.sysds.lops.LopProperties.ExecType;
+import org.apache.sysds.runtime.matrix.data.MatrixValue.CellIndex;
+import org.apache.sysds.test.AutomatedTestBase;
+import org.apache.sysds.test.TestConfiguration;
+
+public class IndexedAdditionAssignmentTest extends AutomatedTestBase
+{
+ private final static String TEST_DIR = "functions/indexing/";
+ private final static String TEST_NAME = "IndexedAdditionTest";
+
+ private final static String TEST_CLASS_DIR = TEST_DIR + IndexedAdditionAssignmentTest.class.getSimpleName() + "/";
+
+ private final static int rows = 1279;
+ private final static int cols = 1050;
+
+ @Override
+ public void setUp() {
+ addTestConfiguration(TEST_NAME, new TestConfiguration(TEST_CLASS_DIR, TEST_NAME, new String[] {"A"}));
+ }
+
+ @Test
+ public void testIndexedAssignmentAddScalarCP() {
+ runIndexedAdditionAssignment(true, ExecType.CP);
+ }
+
+ @Test
+ public void testIndexedAssignmentAddMatrixCP() {
+ runIndexedAdditionAssignment(false, ExecType.CP);
+ }
+
+ @Test
+ public void testIndexedAssignmentAddScalarSpark() {
+ runIndexedAdditionAssignment(true, ExecType.SPARK);
+ }
+
+ @Test
+ public void testIndexedAssignmentAddMatrixSpark() {
+ runIndexedAdditionAssignment(false, ExecType.SPARK);
+ }
+
+ private void runIndexedAdditionAssignment(boolean scalar, ExecType instType) {
+ ExecMode platformOld = setExecMode(instType);
+
+ try {
+ TestConfiguration config = getTestConfiguration(TEST_NAME);
+ loadTestConfiguration(config);
+
+ //test is adding or subtracting 7 to area 1x1 or 10x10
+ //of an initially constraint (3) matrix and sums it up
+ String HOME = SCRIPT_DIR + TEST_DIR;
+ fullDMLScriptName = HOME + TEST_NAME + ".dml";
+ programArgs = new String[]{"-explain" , "-args",
+ Long.toString(rows), Long.toString(cols),
+ String.valueOf(scalar).toUpperCase(), output("A")};
+
+ runTest(true, false, null, -1);
+
+ Double ret = readDMLMatrixFromOutputDir("A").get(new CellIndex(1,1));
+ Assert.assertEquals(new Double(3*rows*cols + 7*(scalar?1:100)), ret);
+ }
+ finally {
+ resetExecMode(platformOld);
+ }
+ }
+}
diff --git a/src/test/java/org/apache/sysds/test/functions/indexing/LeftIndexingScalarTest.java b/src/test/java/org/apache/sysds/test/functions/indexing/LeftIndexingScalarTest.java
index b5ea0aa..68fbc37 100644
--- a/src/test/java/org/apache/sysds/test/functions/indexing/LeftIndexingScalarTest.java
+++ b/src/test/java/org/apache/sysds/test/functions/indexing/LeftIndexingScalarTest.java
@@ -22,7 +22,6 @@
import java.util.HashMap;
import org.junit.Test;
-import org.apache.sysds.api.DMLScript;
import org.apache.sysds.common.Types.ExecMode;
import org.apache.sysds.lops.LopProperties.ExecType;
import org.apache.sysds.runtime.matrix.data.MatrixValue.CellIndex;
@@ -33,7 +32,6 @@
public class LeftIndexingScalarTest extends AutomatedTestBase
{
-
private final static String TEST_DIR = "functions/indexing/";
private final static String TEST_NAME = "LeftIndexingScalarTest";
private final static String TEST_CLASS_DIR = TEST_DIR + LeftIndexingScalarTest.class.getSimpleName() + "/";
@@ -52,31 +50,18 @@
}
@Test
- public void testLeftIndexingScalarCP()
- {
+ public void testLeftIndexingScalarCP() {
runLeftIndexingTest(ExecType.CP);
}
@Test
- public void testLeftIndexingScalarSP()
- {
+ public void testLeftIndexingScalarSP() {
runLeftIndexingTest(ExecType.SPARK);
}
private void runLeftIndexingTest( ExecType instType )
- {
- //rtplatform for MR
- ExecMode platformOld = rtplatform;
- if(instType == ExecType.SPARK) {
- rtplatform = ExecMode.SPARK;
- }
- else {
- rtplatform = ExecMode.HYBRID;
- }
- boolean sparkConfigOld = DMLScript.USE_LOCAL_SPARK_CONFIG;
- if( rtplatform == ExecMode.SPARK )
- DMLScript.USE_LOCAL_SPARK_CONFIG = true;
-
+ {
+ ExecMode platformOld = setExecMode(instType);
try
{
@@ -91,10 +76,10 @@
fullRScriptName = HOME + TEST_NAME + ".R";
rCmd = "Rscript" + " " + fullRScriptName + " " + inputDir() + " " + expectedDir();
- double[][] A = getRandomMatrix(rows, cols, min, max, sparsity, System.currentTimeMillis());
- writeInputMatrix("A", A, true);
-
- runTest(true, false, null, -1);
+ double[][] A = getRandomMatrix(rows, cols, min, max, sparsity, System.currentTimeMillis());
+ writeInputMatrix("A", A, true);
+
+ runTest(true, false, null, -1);
runRScript(true);
HashMap<CellIndex, Double> dmlfile = readDMLMatrixFromOutputDir("A");
@@ -102,11 +87,8 @@
TestUtils.compareMatrices(dmlfile, rfile, epsilon, "A-DML", "A-R");
checkDMLMetaDataFile("A", new MatrixCharacteristics(rows,cols,1,1));
}
- finally
- {
- rtplatform = platformOld;
- DMLScript.USE_LOCAL_SPARK_CONFIG = sparkConfigOld;
+ finally {
+ resetExecMode(platformOld);
}
}
}
-
diff --git a/src/test/scripts/functions/indexing/IndexedAdditionTest.dml b/src/test/scripts/functions/indexing/IndexedAdditionTest.dml
new file mode 100644
index 0000000..415a795
--- /dev/null
+++ b/src/test/scripts/functions/indexing/IndexedAdditionTest.dml
@@ -0,0 +1,31 @@
+#-------------------------------------------------------------
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+#-------------------------------------------------------------
+
+
+A = matrix(3, $1, $2);
+
+if( $3 )
+ A[10,20] += 7;
+else
+ A[10:19,20:29] += 7;
+
+R = as.matrix(sum(A))
+write(R, $4, format="text")
