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: us...@infra.apache.org