Repository: incubator-systemml Updated Branches: refs/heads/master f274c9d3d -> 5bde577a2
[SYSTEMML-673] Fix right-indexing rewrites w/ invalid ranges, tests The right indexing rewrite of Y[a:b,c:d] -> Y iff dims(Y[a:b,c:d]) == dims(Y) is invalid in the presence of invalid indexing ranges as it might hide runtime errors. For now we disable this rewrite for the common scenario of scalar indexing but keep it for matrix indexing due to its performance impact. Down the road, we need to harden the validate to make sure that hops/lops can work against valid programs. This patch also includes fixes of valid range checks of matrix get/set. Project: http://git-wip-us.apache.org/repos/asf/incubator-systemml/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-systemml/commit/2f1d6fa3 Tree: http://git-wip-us.apache.org/repos/asf/incubator-systemml/tree/2f1d6fa3 Diff: http://git-wip-us.apache.org/repos/asf/incubator-systemml/diff/2f1d6fa3 Branch: refs/heads/master Commit: 2f1d6fa3bcb23fe5337f834f646cbe0c271661fa Parents: f274c9d Author: Matthias Boehm <[email protected]> Authored: Sat May 7 20:42:52 2016 -0700 Committer: Matthias Boehm <[email protected]> Committed: Sat May 7 20:44:08 2016 -0700 ---------------------------------------------------------------------- .../java/org/apache/sysml/hops/IndexingOp.java | 3 +- .../RewriteAlgebraicSimplificationDynamic.java | 5 +- .../sysml/runtime/matrix/data/MatrixBlock.java | 4 +- .../UnboundedScalarRightIndexingTest.java | 107 +++++++++++++++++++ .../UnboundedScalarRightIndexingTest.dml | 30 ++++++ .../functions/indexing/ZPackageSuite.java | 3 +- 6 files changed, 147 insertions(+), 5 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-systemml/blob/2f1d6fa3/src/main/java/org/apache/sysml/hops/IndexingOp.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/sysml/hops/IndexingOp.java b/src/main/java/org/apache/sysml/hops/IndexingOp.java index 4b9901c..6a6da5d 100644 --- a/src/main/java/org/apache/sysml/hops/IndexingOp.java +++ b/src/main/java/org/apache/sysml/hops/IndexingOp.java @@ -104,7 +104,8 @@ public class IndexingOp extends Hop //rewrite remove unnecessary right indexing if( dimsKnown() && input.dimsKnown() - && getDim1() == input.getDim1() && getDim2() == input.getDim2() ) + && getDim1() == input.getDim1() && getDim2() == input.getDim2() + && !(getDim1()==1 && getDim2()==1)) { setLops( input.constructLops() ); } http://git-wip-us.apache.org/repos/asf/incubator-systemml/blob/2f1d6fa3/src/main/java/org/apache/sysml/hops/rewrite/RewriteAlgebraicSimplificationDynamic.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/sysml/hops/rewrite/RewriteAlgebraicSimplificationDynamic.java b/src/main/java/org/apache/sysml/hops/rewrite/RewriteAlgebraicSimplificationDynamic.java index 6a0085f..5375539 100644 --- a/src/main/java/org/apache/sysml/hops/rewrite/RewriteAlgebraicSimplificationDynamic.java +++ b/src/main/java/org/apache/sysml/hops/rewrite/RewriteAlgebraicSimplificationDynamic.java @@ -233,9 +233,12 @@ public class RewriteAlgebraicSimplificationDynamic extends HopRewriteRule if( hi instanceof IndexingOp ) //indexing op { Hop input = hi.getInput().get(0); - if( HopRewriteUtils.isEqualSize(hi, input) ) //equal dims + if( HopRewriteUtils.isEqualSize(hi, input) //equal dims + && !(hi.getDim1()==1 && hi.getDim2()==1) ) //not 1-1 matrix { //equal dims of right indexing input and output -> no need for indexing + //(not applied for 1-1 matrices because low potential and issues w/ error + //handling if out of range indexing) //remove unnecessary right indexing HopRewriteUtils.removeChildReference(parent, hi); http://git-wip-us.apache.org/repos/asf/incubator-systemml/blob/2f1d6fa3/src/main/java/org/apache/sysml/runtime/matrix/data/MatrixBlock.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/sysml/runtime/matrix/data/MatrixBlock.java b/src/main/java/org/apache/sysml/runtime/matrix/data/MatrixBlock.java index f62fbc6..c3af41f 100644 --- a/src/main/java/org/apache/sysml/runtime/matrix/data/MatrixBlock.java +++ b/src/main/java/org/apache/sysml/runtime/matrix/data/MatrixBlock.java @@ -651,7 +651,7 @@ public class MatrixBlock extends MatrixValue implements CacheBlock, Externalizab public double getValue(int r, int c) { //matrix bounds check - if( r>rlen || c > clen ) + if( r >= rlen || c >= clen ) throw new RuntimeException("indexes ("+r+","+c+") out of range ("+rlen+","+clen+")"); return quickGetValue(r, c); @@ -661,7 +661,7 @@ public class MatrixBlock extends MatrixValue implements CacheBlock, Externalizab public void setValue(int r, int c, double v) { //matrix bounds check - if( r>rlen || c > clen ) + if( r >= rlen || c >= clen ) throw new RuntimeException("indexes ("+r+","+c+") out of range ("+rlen+","+clen+")"); quickSetValue(r, c, v); http://git-wip-us.apache.org/repos/asf/incubator-systemml/blob/2f1d6fa3/src/test/java/org/apache/sysml/test/integration/functions/indexing/UnboundedScalarRightIndexingTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/sysml/test/integration/functions/indexing/UnboundedScalarRightIndexingTest.java b/src/test/java/org/apache/sysml/test/integration/functions/indexing/UnboundedScalarRightIndexingTest.java new file mode 100644 index 0000000..0c592f5 --- /dev/null +++ b/src/test/java/org/apache/sysml/test/integration/functions/indexing/UnboundedScalarRightIndexingTest.java @@ -0,0 +1,107 @@ +/* + * 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.sysml.test.integration.functions.indexing; + +import org.junit.Test; +import org.apache.sysml.api.DMLException; +import org.apache.sysml.api.DMLScript; +import org.apache.sysml.api.DMLScript.RUNTIME_PLATFORM; +import org.apache.sysml.lops.LopProperties.ExecType; +import org.apache.sysml.test.integration.AutomatedTestBase; +import org.apache.sysml.test.integration.TestConfiguration; + + +public class UnboundedScalarRightIndexingTest extends AutomatedTestBase +{ + private final static String TEST_NAME = "UnboundedScalarRightIndexingTest"; + private final static String TEST_DIR = "functions/indexing/"; + private final static String TEST_CLASS_DIR = TEST_DIR + UnboundedScalarRightIndexingTest.class.getSimpleName() + "/"; + + @Override + public void setUp() { + addTestConfiguration(TEST_NAME, new TestConfiguration(TEST_CLASS_DIR, TEST_NAME, new String[] {})); + } + + @Test + public void testRightIndexingCPNonzero() { + runRightIndexingTest(ExecType.CP, 7); + } + + @Test + public void testRightIndexingSPNonzero() { + runRightIndexingTest(ExecType.SPARK, 7); + } + + @Test + public void testRightIndexingMRNonzero() { + runRightIndexingTest(ExecType.MR, 7); + } + + @Test + public void testRightIndexingCPZero() { + runRightIndexingTest(ExecType.CP, 0); + } + + @Test + public void testRightIndexingSPZero() { + runRightIndexingTest(ExecType.SPARK, 0); + } + + @Test + public void testRightIndexingMRZero() { + runRightIndexingTest(ExecType.MR, 0); + } + + /** + * + * @param et + */ + public void runRightIndexingTest( ExecType et, int val ) + { + RUNTIME_PLATFORM platformOld = rtplatform; + switch( et ){ + case MR: rtplatform = RUNTIME_PLATFORM.HADOOP; break; + case SPARK: rtplatform = RUNTIME_PLATFORM.SPARK; break; + default: rtplatform = RUNTIME_PLATFORM.HYBRID; break; + } + + boolean sparkConfigOld = DMLScript.USE_LOCAL_SPARK_CONFIG; + if( rtplatform == RUNTIME_PLATFORM.SPARK ) + DMLScript.USE_LOCAL_SPARK_CONFIG = true; + + try { + TestConfiguration config = getTestConfiguration(TEST_NAME); + loadTestConfiguration(config); + + String RI_HOME = SCRIPT_DIR + TEST_DIR; + fullDMLScriptName = RI_HOME + TEST_NAME + ".dml"; + programArgs = new String[]{ "-args", String.valueOf(val) }; + fullRScriptName = RI_HOME + TEST_NAME + ".R"; + + //run test (expected runtime exception) + runTest(true, true, DMLException.class, -1); + } + finally + { + rtplatform = platformOld; + DMLScript.USE_LOCAL_SPARK_CONFIG = sparkConfigOld; + } + } +} http://git-wip-us.apache.org/repos/asf/incubator-systemml/blob/2f1d6fa3/src/test/scripts/functions/indexing/UnboundedScalarRightIndexingTest.dml ---------------------------------------------------------------------- diff --git a/src/test/scripts/functions/indexing/UnboundedScalarRightIndexingTest.dml b/src/test/scripts/functions/indexing/UnboundedScalarRightIndexingTest.dml new file mode 100644 index 0000000..8944813 --- /dev/null +++ b/src/test/scripts/functions/indexing/UnboundedScalarRightIndexingTest.dml @@ -0,0 +1,30 @@ +#------------------------------------------------------------- +# +# 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. +# +#------------------------------------------------------------- + + +X = matrix($1, rows=4, cols=3) +Y = X[2,3] + +for (i in 1:100) { + for (j in 1:500) { + print("Y["+i+","+j+"]: " + as.scalar(Y[i,j])) + } +} http://git-wip-us.apache.org/repos/asf/incubator-systemml/blob/2f1d6fa3/src/test_suites/java/org/apache/sysml/test/integration/functions/indexing/ZPackageSuite.java ---------------------------------------------------------------------- diff --git a/src/test_suites/java/org/apache/sysml/test/integration/functions/indexing/ZPackageSuite.java b/src/test_suites/java/org/apache/sysml/test/integration/functions/indexing/ZPackageSuite.java index 521c362..1db03cb 100644 --- a/src/test_suites/java/org/apache/sysml/test/integration/functions/indexing/ZPackageSuite.java +++ b/src/test_suites/java/org/apache/sysml/test/integration/functions/indexing/ZPackageSuite.java @@ -33,7 +33,8 @@ import org.junit.runners.Suite; RightIndexingMatrixTest.class, RightIndexingVectorTest.class, - Jdk7IssueRightIndexingTest.class + Jdk7IssueRightIndexingTest.class, + UnboundedScalarRightIndexingTest.class, })
