systemml-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [systemml] mboehm7 commented on a change in pull request #910: [MINOR] L2SVM, now directly support 1 Hot input
Date Sun, 17 May 2020 20:15:00 GMT

mboehm7 commented on a change in pull request #910:
URL: https://github.com/apache/systemml/pull/910#discussion_r426299182



##########
File path: scripts/builtin/l2svm.dml
##########
@@ -44,58 +47,62 @@
 
 
 m_l2svm = function(Matrix[Double] X, Matrix[Double] Y, Boolean intercept = FALSE,
-Double epsilon = 0.001, Double lambda = 1, Integer maxiterations = 100, Boolean verbose =
FALSE)
+                  Double epsilon = 0.001, Double lambda = 1, Integer max_iterations = 100,

+                  Boolean verbose = FALSE, Integer column_id = -1)

Review comment:
       This Python-like indentation is a bit unpleasant to the eye.

##########
File path: scripts/builtin/confusionMatrix.dml
##########
@@ -0,0 +1,51 @@
+#-------------------------------------------------------------
+#
+# 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.
+#
+#-------------------------------------------------------------
+
+# INPUT PARAMETERS:
+# ---------------------------------------------------------------------------------------------
+# NAME            TYPE    DEFAULT     MEANING
+# ---------------------------------------------------------------------------------------------
+# P               Double  ---         matrix of Predictions
+# Y               Double  ---         matrix of Golden standard One Hot Encoded
+# ---------------------------------------------------------------------------------------------
+# OUTPUT:
+# ---------------------------------------------------------------------------------------------
+# NAME            TYPE    DEFAULT     MEANING
+# ---------------------------------------------------------------------------------------------
+# ConfusionMatrix Double  ---         The Confusion Matrix
+
+
+m_confusionMatrix = function(Matrix[Double] P, Matrix[Double] Y)
+  return(Matrix[Double] confusion)
+{
+  if(nrow(P) != nrow(Y))
+    stop("CONFUSION MATRIX: The number of rows have to be equal in both P ["+nrow(P)+"] and
Y ["+nrow(Y)+"]")
+
+  res = matrix(0, max(P), max(Y))
+  for(i in 1:nrow(P) ){
+    row = as.scalar(Y[i,1])
+    col = as.scalar(P[i,1])
+
+    res[row,col] = as.scalar(res[row,col]) +  1
+  }

Review comment:
       Please replace the entire for loop with a `table(P[1,], Y[1,])` (it's fine to have
a remapping to table). Also note that the construction of `res` use inconsistent lhs/rhs matrices
(P/Y vs Y/P).

##########
File path: scripts/builtin/l2svm.dml
##########
@@ -112,32 +119,37 @@ Double epsilon = 0.001, Double lambda = 1, Integer maxiterations = 100,
Boolean
       step_sz = step_sz - g/h
       continue1 = (g*g/h >= epsilon)
     }
-  
+
     #update weights
     w = w + step_sz*s
     Xw = Xw + step_sz*Xd
-  
+
     out = 1 - Y * Xw
     sv = (out > 0)
     out = sv * out
     obj = 0.5 * sum(out * out) + lambda/2 * sum(w * w)
     g_new = t(X) %*% (out * Y) - lambda * w
-  
-    
-    if(verbose) print("Iter, Obj "+ iter + ", "+obj)
-  
+
+    if(verbose) {
+      if(column_id != -1){
+        print("Iter:" + toString(iter) + ", Col:" + column_id + ", Obj:" + obj)
+      } 
+      else {
+        print("Iter:" + toString(iter) + ", Obj:" + obj)
+      }
+    } 
+
     tmp = sum(s * g_old)
     if(step_sz*tmp < epsilon*obj){
       continue = 0
     }

Review comment:
       Somehow I missed this on the initial commit, but this can become: `continue = (step_sz*tmp
>= epsilon*obj & sum(s^2) != 0);`

##########
File path: scripts/builtin/l2svm.dml
##########
@@ -112,32 +119,37 @@ Double epsilon = 0.001, Double lambda = 1, Integer maxiterations = 100,
Boolean
       step_sz = step_sz - g/h
       continue1 = (g*g/h >= epsilon)
     }
-  
+
     #update weights
     w = w + step_sz*s
     Xw = Xw + step_sz*Xd
-  
+
     out = 1 - Y * Xw
     sv = (out > 0)
     out = sv * out
     obj = 0.5 * sum(out * out) + lambda/2 * sum(w * w)
     g_new = t(X) %*% (out * Y) - lambda * w
-  
-    
-    if(verbose) print("Iter, Obj "+ iter + ", "+obj)
-  
+
+    if(verbose) {
+      if(column_id != -1){
+        print("Iter:" + toString(iter) + ", Col:" + column_id + ", Obj:" + obj)
+      } 
+      else {
+        print("Iter:" + toString(iter) + ", Obj:" + obj)
+      }

Review comment:
       could we maybe replace this with something more concise like:
   ```
   if(verbose) {
     colstr = ifelse(column_id!=-1, ", Col:"+column_id, "")
     print("Iter:" + toString(iter) + colstr + ", Obj:" + obj)
   }
   ```

##########
File path: scripts/builtin/msvm.dml
##########
@@ -42,34 +43,45 @@
 # ---------------------------------------------------------------------------------------------
 # model           Double   ---        model matrix
 
-m_msvm = function(Matrix[Double] X, Matrix[Double] Y, Boolean intercept = FALSE, Integer
num_classes =10,
+m_msvm = function(Matrix[Double] X, Matrix[Double] Y, Boolean intercept = FALSE,
                   Double epsilon = 0.001, Double lambda = 1.0, Integer max_iterations = 100,
Boolean verbose = FALSE)
   return(Matrix[Double] model)
 {
   if(verbose)
     print("Built-in Multiclass-SVM started")
 
-  num_samples = nrow(X)
-  num_features = ncol(X)
-
-  num_rows_in_w = num_features
+  num_rows_in_w = ncol(X)
   if(intercept) {
     num_rows_in_w = num_rows_in_w + 1
   }
-  
-  w = matrix(0, rows=num_rows_in_w, cols=num_classes)
 
-  parfor(iter_class in 1:num_classes) {
-    Y_local = 2 * (Y == iter_class) - 1
-    if(verbose) {
-      print("iter class: " + iter_class)
-      print("y local: " + toString(Y_local))
+  if(ncol(Y) > 1){
+    w = matrix(0, rows=num_rows_in_w, cols=ncol(Y))
+    # If multiple columns, assume the input to be OneHotEncoded already.
+    if((min(Y) != 0 | min(Y) != -1) & max(Y) != 1)
+      stop("MSVM: Invalid OneHot input matrix Y")
+    
+    parfor(class in 1:ncol(Y)){
+      w[,class] = l2svm(X=X, Y=Y[,class], intercept=intercept,
+          epsilon=epsilon, lambda=lambda, max_iterations=max_iterations, 
+          verbose= verbose, column_id=class)
     }

Review comment:
       Instead of replicating this code of running the core of msvm, please simply modify
Y as follows:
   ```
   if( ncol(Y) > 1 )
     Y = rowMax(Y * t(seq(1,ncol(Y))))
   ```

##########
File path: src/test/java/org/apache/sysds/test/functions/builtin/BuiltinMulticlassSVMTest.java
##########
@@ -99,16 +98,16 @@ private void runMSVMTest(boolean sparse, boolean  intercept, int classes,
double
 			double sparsity = sparse ? spSparse : spDense;
 			String HOME = SCRIPT_DIR + TEST_DIR;
 			fullDMLScriptName = HOME + TEST_NAME + ".dml";
-			programArgs = new String[]{ "-explain", "-stats",
+			programArgs = new String[]{
 				"-nvargs", "X=" + input("X"), "Y=" + input("Y"), "model=" + output("model"),
-				"inc=" + String.valueOf(intercept).toUpperCase(), "num_classes=" + classes, "eps=" +
eps, "lam=" + lambda, "max=" + run};
+				"inc=" + String.valueOf(intercept).toUpperCase(), "eps=" + eps, "lam=" + lambda, "max="
+ run};
 
 			fullRScriptName = HOME + TEST_NAME + ".R";
-			rCmd = getRCmd(inputDir(), Boolean.toString(intercept), Integer.toString(classes),  Double.toString(eps),
+			rCmd = getRCmd(inputDir(), Boolean.toString(intercept),  Double.toString(eps),

Review comment:
       The test is currently failing because the removed parameter shifts the output filename
from position 7 to 6 but the R script is not updated consistently.

##########
File path: src/test/java/org/apache/sysds/test/functions/jmlc/ReuseModelVariablesTest.java
##########
@@ -35,7 +35,7 @@
 public class ReuseModelVariablesTest extends AutomatedTestBase 
 {
 	private final static String TEST_NAME1 = "reuse-glm-predict";
-	private final static String TEST_NAME2 = "reuse-msvm-predict";
+	private final static String TEST_NAME2 = "reuse--predict";

Review comment:
       why?

##########
File path: src/test/java/org/apache/sysds/test/functions/builtin/BuiltinMulticlassSVMTest.java
##########
@@ -53,38 +52,38 @@ public void setUp() {
 
 	@Test
 	public void testMSVMDense() {
-		runMSVMTest(false, false, num_classes, eps, 1.0, max_iter, LopProperties.ExecType.CP);
+		runMSVMTest(false, false, eps, 1.0, max_iter, LopProperties.ExecType.CP);
 	}
 	@Test
 	public void testMSVMSparse() {
-		runMSVMTest(true, false, num_classes, eps, 1.0, max_iter, LopProperties.ExecType.CP);
+		runMSVMTest(true, false, eps, 1.0, max_iter, LopProperties.ExecType.CP);
 	}
 	@Test
 	public void testMSVMInterceptSpark() {
-		runMSVMTest(true,true, num_classes, eps, 1.0, max_iter, LopProperties.ExecType.SPARK);
+		runMSVMTest(true,true, eps, 1.0, max_iter, LopProperties.ExecType.SPARK);
 	}
 
 	@Test
 	public void testMSVMSparseLambda2() {
-		runMSVMTest(true,true, num_classes, eps,2.0, max_iter, LopProperties.ExecType.CP);
+		runMSVMTest(true,true, eps,2.0, max_iter, LopProperties.ExecType.CP);
 	}
 	@Test
 	public void testMSVMSparseLambda100CP() {
-		runMSVMTest(true,true, num_classes, 1, 100, max_iter, LopProperties.ExecType.CP);
+		runMSVMTest(true,true, 1, 100, max_iter, LopProperties.ExecType.CP);
 	}
 	@Test
 	public void testMSVMSparseLambda100Spark() {
-		runMSVMTest(true,true, num_classes, 1, 100, max_iter, LopProperties.ExecType.SPARK);
+		runMSVMTest(true,true, 1, 100, max_iter, LopProperties.ExecType.SPARK);
 	}
 	@Test
 	public void testMSVMIteration() {
-		runMSVMTest(true,true, num_classes, 1, 2.0, 100, LopProperties.ExecType.CP);
+		runMSVMTest(true,true, 1, 2.0, 100, LopProperties.ExecType.CP);
 	}
 	@Test
 	public void testMSVMDenseIntercept() {
-		runMSVMTest(false,true, num_classes, eps, 1.0, max_iter, LopProperties.ExecType.CP);
+		runMSVMTest(false,true, eps, 1.0, max_iter, LopProperties.ExecType.CP);
 	}
-	private void runMSVMTest(boolean sparse, boolean  intercept, int classes, double eps,
+	private void runMSVMTest(boolean sparse, boolean  intercept, double eps,
 							  double lambda, int run, LopProperties.ExecType instType)

Review comment:
       again, personally I would use a single indentation level for parameters (as people
read from left to right) but I don't have a strong opinion here.




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



Mime
View raw message