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 2020/07/21 14:41:34 UTC
[GitHub] [systemds] kev-inn commented on a change in pull request #984: [SYSTEMDS-2540] Stop Exception and Test improvements
kev-inn commented on a change in pull request #984:
URL: https://github.com/apache/systemds/pull/984#discussion_r458140120
##########
File path: src/test/java/org/apache/sysds/test/applications/GLMTest.java
##########
@@ -140,7 +140,7 @@ public GLMTest (int numRecords_, int numFeatures_, int distFamilyType_, double d
// THIS IS TO TEST "INTERCEPT AND SHIFT/SCALE" OPTION ("icpt=2"):
- { 200000, 50, 1, 0.0, 1, 0.0, 0.01, 3.0, 10.0, 2.0, 2.5 }, // Gaussian.log // CHECK DEVIANCE !!!
+ // { 200000, 50, 1, 0.0, 1, 0.0, 0.01, 3.0, 10.0, 2.0, 2.5 }, // Gaussian.log // CHECK DEVIANCE !!!
Review comment:
Since a fix is probably of interest, maybe a TODO would be appropriate?
##########
File path: scripts/algorithms/Kmeans.dml
##########
@@ -188,9 +187,9 @@ print ("Number of successful runs = " + as.integer (as.scalar (termination_stats
print ("Number of incomplete runs = " + as.integer (as.scalar (termination_stats [1, 2])));
print ("Number of failed runs (with lost centroids) = " + as.integer (as.scalar (termination_stats [1, 3])));
-num_successful_runs = as.scalar (termination_stats [1, 1]);
+num_successful_runs = as.integer(as.scalar (termination_stats [1, 1]));
if (num_successful_runs > 0) {
- final_wcss_successful = final_wcss * termination_bitmap [, 1];
+ final_wcss_successful = replace(target = final_wcss, pattern = Inf, replacement = 0)* termination_bitmap [, 1];
Review comment:
LGTM
##########
File path: src/test/java/org/apache/sysds/test/functions/codegenalg/partone/AlgorithmKMeans.java
##########
@@ -39,14 +39,14 @@
//private final static double eps = 1e-5;
- private final static int rows = 2972;
- private final static int cols = 972;
+ private final static int rows = 1241;
+ private final static int cols = 83;
private final static double sparsity1 = 0.7; //dense
private final static double sparsity2 = 0.1; //sparse
private final static double epsilon = 0.000000001;
- private final static double maxiter = 10;
+ private final static double maxiter = 50;
Review comment:
What is the reason for changed params? Speed?
##########
File path: src/test/java/org/apache/sysds/test/functions/builtin/BuiltinKmeansTest.java
##########
@@ -28,17 +28,21 @@
import org.apache.sysds.test.TestConfiguration;
import org.apache.sysds.test.TestUtils;
+
+/**
+ * TODO FIX Stability. The test currently sometimes fails due to differences in test executions and random behaviour in operations.
+*/
public class BuiltinKmeansTest extends AutomatedTestBase
{
private final static String TEST_NAME = "kmeans";
private final static String TEST_DIR = "functions/builtin/";
private static final String TEST_CLASS_DIR = TEST_DIR + BuiltinKmeansTest.class.getSimpleName() + "/";
private final static double eps = 1e-10;
- private final static int rows = 3972;
- private final static int cols = 972;
+ private final static int rows = 1320;
+ private final static int cols = 32;
private final static double spSparse = 0.3;
private final static double spDense = 0.7;
- private final static double max_iter = 10;
+ private final static double max_iter = 50;
Review comment:
Again, what is the reason for the param change? Just out of curiosity.
##########
File path: scripts/algorithms/StepGLM.dml
##########
@@ -173,15 +173,15 @@ if (dir == "forward") {
# Subsequent passes over the features
parfor (i in 1:num_features) {
if (as.scalar(columns_fixed[1,i]) == 0) {
-
+
# Construct the feature matrix
X = cbind (X_global, X_orig[,i]);
-
Review comment:
Are tabs our standard for sure? I see a lot of DML files with spaces.
##########
File path: src/test/java/org/apache/sysds/test/functions/misc/ConditionalValidateTest.java
##########
@@ -55,52 +55,52 @@ public void setUp() {
@Test
public void testUnconditionalReadNoError()
{
- runTest( TEST_NAME1, false, true );
+ runTest( TEST_NAME1, null, true );
}
@Test
public void testUnconditionalReadError()
{
- runTest( TEST_NAME1, true, false );
+ runTest( TEST_NAME1, LanguageException.class, false );
}
@Test
public void testIfConditionalReadNoErrorExists()
{
- runTest( TEST_NAME2, false, true );
+ runTest( TEST_NAME2, null, true );
}
@Test
public void testIfConditionalReadNoErrorNotExists()
{
- runTest( TEST_NAME2, false, false );
+ runTest( TEST_NAME2, null, false );
}
@Test
public void testForConditionalReadNoErrorExists()
{
- runTest( TEST_NAME3, false, true );
+ runTest( TEST_NAME3, null, true );
}
@Test
public void testForConditionalReadNoErrorNotExists()
{
- runTest( TEST_NAME3, false, false );
+ runTest( TEST_NAME3, null, false );
}
@Test
public void testWhileConditionalReadNoErrorExists()
{
- runTest( TEST_NAME4, false, true );
+ runTest( TEST_NAME4, null, true );
}
@Test
public void testWhileConditionalReadNoErrorNotExists()
{
- runTest( TEST_NAME4, false, false );
+ runTest( TEST_NAME4, null, false );
}
- private void runTest( String testName, boolean exceptionExpected, boolean fileExists )
+ private void runTest( String testName, Class<?> exceptionExpected, boolean fileExists )
Review comment:
Variable name
##########
File path: src/test/java/org/apache/sysds/test/functions/misc/DataTypeCastingTest.java
##########
@@ -85,10 +85,10 @@ public void testMatrixToMatrix()
* @param cfc
* @param vt
*/
- private void runTest( String testName, boolean matrixInput, boolean exceptionExpected )
+ private void runTest( String testName, boolean matrixInput, Class<?> exceptionExpected )
Review comment:
A variable name change would be good, maybe just `exception`?
##########
File path: src/main/java/org/apache/sysds/hops/rewrite/RewriteConstantFolding.java
##########
@@ -98,13 +98,7 @@ private Hop rConstantFoldingExpression( Hop root ) {
if( root.getDataType() == DataType.SCALAR //scalar output
&& ( isApplicableBinaryOp(root) || isApplicableUnaryOp(root) ) )
{
- //core constant folding via runtime instructions
- try {
- literal = evalScalarOperation(root);
- }
- catch(Exception ex) {
- LOG.error("Failed to execute constant folding instructions. No abort.", ex);
- }
+ literal = evalScalarOperation(root);
Review comment:
It would anyway fail at some point not only if it tries to rewrite, right? Did you check if the error message still is informative enough if it fails here?
##########
File path: src/test/java/org/apache/sysds/test/functions/io/csv/ReadCSVTest1.java
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.io.csv;
+
+public class ReadCSVTest1 extends ReadCSVTest {
+
+ private final static String TEST_NAME = "ReadCSVTest";
+ private final static String TEST_CLASS_DIR = TEST_DIR + ReadCSVTest1.class.getSimpleName() + "/";
+
+ protected String getTestName() {
+ return TEST_NAME;
+ }
+
+ protected String getTestClassDir() {
+ return TEST_CLASS_DIR;
+ }
+
+ protected int getId() {
+ return 1;
+ }
+}
Review comment:
I preferred the old version, although it could have used arrays for the IDs/test_numbers and parameterized tests. It comes down to personal preference tough. Same with the other tests.
##########
File path: src/test/java/org/apache/sysds/test/AutomatedTestBase.java
##########
@@ -978,19 +989,26 @@ protected void runRScript(boolean newWay) {
}
long t1 = System.nanoTime();
- System.out.println("R is finished (in " + ((double) t1 - t0) / 1000000000 + " sec)");
+
+ LOG.info("R is finished (in " + ((double) t1 - t0) / 1000000000 + " sec)");
}
catch(Exception e) {
- e.printStackTrace();
- StringBuilder errorMessage = new StringBuilder();
- errorMessage.append("failed to run script " + executionFile);
- errorMessage.append("\nexception: " + e.toString());
- errorMessage.append("\nmessage: " + e.getMessage());
- errorMessage.append("\nstack trace:");
- for(StackTraceElement ste : e.getStackTrace()) {
- errorMessage.append("\n>" + ste);
+ if(e.getMessage().contains("ERROR: R has ended irregularly")){
+ StringBuilder errorMessage = new StringBuilder();
+ errorMessage.append(e.getMessage());
+ fail(errorMessage.toString());
+ }else {
+ e.printStackTrace();
+ StringBuilder errorMessage = new StringBuilder();
+ errorMessage.append("failed to run script " + executionFile);
+ errorMessage.append("\nexception: " + e.toString());
+ errorMessage.append("\nmessage: " + e.getMessage());
+ errorMessage.append("\nstack trace:");
+ for(StackTraceElement ste : e.getStackTrace()) {
+ errorMessage.append("\n>" + ste);
+ }
Review comment:
Why do we use `getStackTraceString` at line 1194, but here we don't?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org