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 2021/01/11 00:14:02 UTC
[GitHub] [systemds] OlgaOvcharenko opened a new pull request #1150: [SYSTEMDS-2766] Federated covariance
OlgaOvcharenko opened a new pull request #1150:
URL: https://github.com/apache/systemds/pull/1150
This PR adds federated covariance instruction.
----------------------------------------------------------------
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
[GitHub] [systemds] OlgaOvcharenko commented on pull request #1150: [SYSTEMDS-2766] Federated covariance
Posted by GitBox <gi...@apache.org>.
OlgaOvcharenko commented on pull request #1150:
URL: https://github.com/apache/systemds/pull/1150#issuecomment-787046164
@Baunsgaard I cleaned the code, can you take another look? Thanks.
----------------------------------------------------------------
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
[GitHub] [systemds] Baunsgaard commented on pull request #1150: [SYSTEMDS-2766] Federated covariance
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on pull request #1150:
URL: https://github.com/apache/systemds/pull/1150#issuecomment-787047218
LGTM, will merge shortly
----------------------------------------------------------------
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
[GitHub] [systemds] Baunsgaard commented on a change in pull request #1150: [SYSTEMDS-2766] Federated covariance
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on a change in pull request #1150:
URL: https://github.com/apache/systemds/pull/1150#discussion_r556507913
##########
File path: src/main/java/org/apache/sysds/runtime/instructions/fed/CovarianceFEDInstruction.java
##########
@@ -0,0 +1,253 @@
+/*
+ * 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.runtime.instructions.fed;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.concurrent.Future;
+
+import org.apache.sysds.common.Types;
+import org.apache.sysds.runtime.DMLRuntimeException;
+import org.apache.sysds.runtime.controlprogram.caching.MatrixObject;
+import org.apache.sysds.runtime.controlprogram.context.ExecutionContext;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedRange;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedRequest;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedResponse;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedUDF;
+import org.apache.sysds.runtime.controlprogram.federated.FederationMap;
+import org.apache.sysds.runtime.controlprogram.federated.FederationUtils;
+import org.apache.sysds.runtime.functionobjects.COV;
+import org.apache.sysds.runtime.instructions.InstructionUtils;
+import org.apache.sysds.runtime.instructions.cp.CM_COV_Object;
+import org.apache.sysds.runtime.instructions.cp.CPOperand;
+import org.apache.sysds.runtime.instructions.cp.Data;
+import org.apache.sysds.runtime.instructions.cp.DoubleObject;
+import org.apache.sysds.runtime.instructions.cp.ScalarObject;
+import org.apache.sysds.runtime.matrix.data.MatrixBlock;
+import org.apache.sysds.runtime.matrix.operators.COVOperator;
+import org.apache.sysds.runtime.matrix.operators.Operator;
+
+public class CovarianceFEDInstruction extends BinaryFEDInstruction {
+ private CovarianceFEDInstruction(Operator op, CPOperand in1, CPOperand in2, CPOperand out, String opcode,
+ String istr) {
+ super(FEDInstruction.FEDType.AggregateBinary, op, in1, in2, out, opcode, istr);
+ }
+
+ private CovarianceFEDInstruction(Operator op, CPOperand in1, CPOperand in2, CPOperand in3, CPOperand out,
+ String opcode, String istr) {
+ super(FEDInstruction.FEDType.AggregateBinary, op, in1, in2, in3, out, opcode, istr);
+ }
+
+
+ public static CovarianceFEDInstruction parseInstruction(String str) {
+ CPOperand in1 = new CPOperand("", Types.ValueType.UNKNOWN, Types.DataType.UNKNOWN);
+ CPOperand in2 = new CPOperand("", Types.ValueType.UNKNOWN, Types.DataType.UNKNOWN);
+ CPOperand in3 = null;
+ CPOperand out = new CPOperand("", Types.ValueType.UNKNOWN, Types.DataType.UNKNOWN);
+
+ String[] parts = InstructionUtils.getInstructionPartsWithValueType(str);
+ String opcode = parts[0];
+
+ if( !opcode.equalsIgnoreCase("cov") ) {
+ throw new DMLRuntimeException("CovarianceCPInstruction.parseInstruction():: Unknown opcode " + opcode);
+ }
+
+ COVOperator cov = new COVOperator(COV.getCOMFnObject());
+ if ( parts.length == 4 ) {
+ parseBinaryInstruction(str, in1, in2, out);
+ return new CovarianceFEDInstruction(cov, in1, in2, out, opcode, str);
+ } else if ( parts.length == 5 ) {
+ in3 = new CPOperand("", Types.ValueType.UNKNOWN, Types.DataType.UNKNOWN);
+ parseBinaryInstruction(str, in1, in2, in3, out);
+ return new CovarianceFEDInstruction(cov, in1, in2, in3, out, opcode, str);
+ }
+ else {
+ throw new DMLRuntimeException("Invalid number of arguments in Instruction: " + str);
+ }
+ }
+
+ @Override
+ public void processInstruction(ExecutionContext ec) {
+ MatrixObject mo1 = ec.getMatrixObject(input1);
+ MatrixObject mo2 = ec.getMatrixObject(input2);
+ COVOperator cop = ((COVOperator)_optr);
+
+ if(mo1.isFederated() && mo2.isFederated() && !mo1.getFedMapping().isAligned(mo2.getFedMapping(), false))
+ throw new DMLRuntimeException("Not supported matrix-matrix binary operation: covariance.");
+
+ FederatedRequest fr2 = null;
+ if(mo1.isFederated() && mo2.isFederated() && mo1.getFedMapping().isAligned(mo2.getFedMapping(), false)) {
+ fr2 = FederationUtils.callInstruction(instString, output, new CPOperand[]{input1, input2},
+ new long[]{mo1.getFedMapping().getID(), mo2.getFedMapping().getID()});
+ Future<FederatedResponse>[] tmp = mo1.getFedMapping().execute(getTID(), true, fr2);
+
+ //means
+ Future<FederatedResponse>[] meanTmp1 = processMean(mo1, 0);
+ Future<FederatedResponse>[] meanTmp2 = processMean(mo2, 1);
+
+ double res = aggCov(tmp, meanTmp1, meanTmp2, mo1.getFedMapping());
+ ec.setVariable(output.getName(), new DoubleObject(res));
+ }
+ else {
+ MatrixBlock mb;
+ MatrixObject mo;
+ if(!mo1.isFederated() && mo2.isFederated()) {
+ mo = mo2;
+ mb = ec.getMatrixInput(input1.getName());
+ }
+ else {
+ mo = mo1;
+ mb = ec.getMatrixInput(input2.getName());
+ }
+
+ FederationMap fedMapping = mo.getFedMapping();
+ List<CM_COV_Object> globalCmobj = new ArrayList<>();
+ long varID = FederationUtils.getNextFedDataID();
+
+ fedMapping.mapParallel(varID, (range, data) -> {
+
+ FederatedResponse response;
+ try {
+ if(input3 == null) {
+ response = data.executeFederatedOperation(new FederatedRequest(FederatedRequest.RequestType.EXEC_UDF,
+ -1,
+ new CovarianceFEDInstruction.COVFunction(data.getVarID(),
+ mb.slice(range.getBeginDimsInt()[0], range.getEndDimsInt()[0] - 1),
+ cop))).get();
+ }
+ else {
+ MatrixBlock wtBlock = ec.getMatrixInput(input2.getName());
+
+ response = data.executeFederatedOperation(new FederatedRequest(FederatedRequest.RequestType.EXEC_UDF,
+ -1,
+ new CovarianceFEDInstruction.COVWeightsFunction(data.getVarID(),
+ mb.slice(range.getBeginDimsInt()[0], range.getEndDimsInt()[0] - 1),
+ cop,
+ wtBlock))).get();
+ }
+
+ if(!response.isSuccessful())
+ response.throwExceptionFromResponse();
+ synchronized(globalCmobj) {
+ globalCmobj.add((CM_COV_Object) response.getData()[0]);
+ }
+ }
+ catch(Exception e) {
+ throw new DMLRuntimeException(e);
+ }
+ return null;
+ });
+
+ Optional<CM_COV_Object> res = globalCmobj.stream().reduce((arg0, arg1) -> (CM_COV_Object) cop.fn.execute(arg0, arg1));
+ try {
+ ec.setScalarOutput(output.getName(), new DoubleObject(res.get().getRequiredResult(cop)));
+ }
+ catch(Exception e) {
+ throw new DMLRuntimeException(e);
+ }
+ }
+ }
+
+ private static class COVFunction extends FederatedUDF {
+
+ private static final long serialVersionUID = -501036588060113499L;
+ private final MatrixBlock _mo2;
+ private final COVOperator _op;
+
+ public COVFunction (long input, MatrixBlock mo2, COVOperator op) {
+ super(new long[] {input});
+ _op = op;
+ _mo2 = mo2;
+ }
+
+ @Override
+ public FederatedResponse execute(ExecutionContext ec, Data... data) {
+ MatrixBlock mb = ((MatrixObject) data[0]).acquireReadAndRelease();
+ return new FederatedResponse(FederatedResponse.ResponseType.SUCCESS, mb.covOperations(_op, _mo2));
+ }
+ }
+
+ private static class COVWeightsFunction extends FederatedUDF {
+ private static final long serialVersionUID = -1768739786192949573L;
+ private final COVOperator _op;
+ private final MatrixBlock _mo2;
+ private final MatrixBlock _weights;
+
+ protected COVWeightsFunction(long input, MatrixBlock mo2, COVOperator op, MatrixBlock weights) {
+ super(new long[] {input});
+ _mo2 = mo2;
+ _op = op;
+ _weights = weights;
+ }
+
+ @Override
+ public FederatedResponse execute(ExecutionContext ec, Data... data) {
+ MatrixBlock mb = ((MatrixObject) data[0]).acquireReadAndRelease();
+ return new FederatedResponse(FederatedResponse.ResponseType.SUCCESS, mb.covOperations(_op, _mo2, _weights));
+ }
+ }
+
+ private static double aggCov(Future<FederatedResponse>[] ffr, Future<FederatedResponse>[] mean1Ffr, Future<FederatedResponse>[] mean2Ffr, FederationMap map) {
Review comment:
Please split this method up. into at least:
1. that gets the federated responses.
2. that calculates the covariance (using only double inputs.)
##########
File path: src/main/java/org/apache/sysds/runtime/instructions/fed/CovarianceFEDInstruction.java
##########
@@ -0,0 +1,253 @@
+/*
+ * 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.runtime.instructions.fed;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.concurrent.Future;
+
+import org.apache.sysds.common.Types;
+import org.apache.sysds.runtime.DMLRuntimeException;
+import org.apache.sysds.runtime.controlprogram.caching.MatrixObject;
+import org.apache.sysds.runtime.controlprogram.context.ExecutionContext;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedRange;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedRequest;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedResponse;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedUDF;
+import org.apache.sysds.runtime.controlprogram.federated.FederationMap;
+import org.apache.sysds.runtime.controlprogram.federated.FederationUtils;
+import org.apache.sysds.runtime.functionobjects.COV;
+import org.apache.sysds.runtime.instructions.InstructionUtils;
+import org.apache.sysds.runtime.instructions.cp.CM_COV_Object;
+import org.apache.sysds.runtime.instructions.cp.CPOperand;
+import org.apache.sysds.runtime.instructions.cp.Data;
+import org.apache.sysds.runtime.instructions.cp.DoubleObject;
+import org.apache.sysds.runtime.instructions.cp.ScalarObject;
+import org.apache.sysds.runtime.matrix.data.MatrixBlock;
+import org.apache.sysds.runtime.matrix.operators.COVOperator;
+import org.apache.sysds.runtime.matrix.operators.Operator;
+
+public class CovarianceFEDInstruction extends BinaryFEDInstruction {
+ private CovarianceFEDInstruction(Operator op, CPOperand in1, CPOperand in2, CPOperand out, String opcode,
+ String istr) {
+ super(FEDInstruction.FEDType.AggregateBinary, op, in1, in2, out, opcode, istr);
+ }
+
+ private CovarianceFEDInstruction(Operator op, CPOperand in1, CPOperand in2, CPOperand in3, CPOperand out,
+ String opcode, String istr) {
+ super(FEDInstruction.FEDType.AggregateBinary, op, in1, in2, in3, out, opcode, istr);
+ }
+
+
+ public static CovarianceFEDInstruction parseInstruction(String str) {
+ CPOperand in1 = new CPOperand("", Types.ValueType.UNKNOWN, Types.DataType.UNKNOWN);
+ CPOperand in2 = new CPOperand("", Types.ValueType.UNKNOWN, Types.DataType.UNKNOWN);
+ CPOperand in3 = null;
+ CPOperand out = new CPOperand("", Types.ValueType.UNKNOWN, Types.DataType.UNKNOWN);
+
+ String[] parts = InstructionUtils.getInstructionPartsWithValueType(str);
+ String opcode = parts[0];
+
+ if( !opcode.equalsIgnoreCase("cov") ) {
+ throw new DMLRuntimeException("CovarianceCPInstruction.parseInstruction():: Unknown opcode " + opcode);
+ }
+
+ COVOperator cov = new COVOperator(COV.getCOMFnObject());
+ if ( parts.length == 4 ) {
+ parseBinaryInstruction(str, in1, in2, out);
+ return new CovarianceFEDInstruction(cov, in1, in2, out, opcode, str);
+ } else if ( parts.length == 5 ) {
+ in3 = new CPOperand("", Types.ValueType.UNKNOWN, Types.DataType.UNKNOWN);
+ parseBinaryInstruction(str, in1, in2, in3, out);
+ return new CovarianceFEDInstruction(cov, in1, in2, in3, out, opcode, str);
+ }
+ else {
+ throw new DMLRuntimeException("Invalid number of arguments in Instruction: " + str);
+ }
+ }
+
+ @Override
+ public void processInstruction(ExecutionContext ec) {
+ MatrixObject mo1 = ec.getMatrixObject(input1);
+ MatrixObject mo2 = ec.getMatrixObject(input2);
+ COVOperator cop = ((COVOperator)_optr);
+
+ if(mo1.isFederated() && mo2.isFederated() && !mo1.getFedMapping().isAligned(mo2.getFedMapping(), false))
+ throw new DMLRuntimeException("Not supported matrix-matrix binary operation: covariance.");
+
+ FederatedRequest fr2 = null;
+ if(mo1.isFederated() && mo2.isFederated() && mo1.getFedMapping().isAligned(mo2.getFedMapping(), false)) {
Review comment:
This if else can also be put into individual methods.
because just looking at this, i have to figure out what is inside each if statements, while if you have methods it would be more explanatory.
##########
File path: src/main/java/org/apache/sysds/runtime/instructions/fed/CovarianceFEDInstruction.java
##########
@@ -0,0 +1,253 @@
+/*
+ * 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.runtime.instructions.fed;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.concurrent.Future;
+
+import org.apache.sysds.common.Types;
+import org.apache.sysds.runtime.DMLRuntimeException;
+import org.apache.sysds.runtime.controlprogram.caching.MatrixObject;
+import org.apache.sysds.runtime.controlprogram.context.ExecutionContext;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedRange;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedRequest;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedResponse;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedUDF;
+import org.apache.sysds.runtime.controlprogram.federated.FederationMap;
+import org.apache.sysds.runtime.controlprogram.federated.FederationUtils;
+import org.apache.sysds.runtime.functionobjects.COV;
+import org.apache.sysds.runtime.instructions.InstructionUtils;
+import org.apache.sysds.runtime.instructions.cp.CM_COV_Object;
+import org.apache.sysds.runtime.instructions.cp.CPOperand;
+import org.apache.sysds.runtime.instructions.cp.Data;
+import org.apache.sysds.runtime.instructions.cp.DoubleObject;
+import org.apache.sysds.runtime.instructions.cp.ScalarObject;
+import org.apache.sysds.runtime.matrix.data.MatrixBlock;
+import org.apache.sysds.runtime.matrix.operators.COVOperator;
+import org.apache.sysds.runtime.matrix.operators.Operator;
+
+public class CovarianceFEDInstruction extends BinaryFEDInstruction {
+ private CovarianceFEDInstruction(Operator op, CPOperand in1, CPOperand in2, CPOperand out, String opcode,
+ String istr) {
+ super(FEDInstruction.FEDType.AggregateBinary, op, in1, in2, out, opcode, istr);
+ }
+
+ private CovarianceFEDInstruction(Operator op, CPOperand in1, CPOperand in2, CPOperand in3, CPOperand out,
+ String opcode, String istr) {
+ super(FEDInstruction.FEDType.AggregateBinary, op, in1, in2, in3, out, opcode, istr);
+ }
+
+
+ public static CovarianceFEDInstruction parseInstruction(String str) {
+ CPOperand in1 = new CPOperand("", Types.ValueType.UNKNOWN, Types.DataType.UNKNOWN);
+ CPOperand in2 = new CPOperand("", Types.ValueType.UNKNOWN, Types.DataType.UNKNOWN);
+ CPOperand in3 = null;
+ CPOperand out = new CPOperand("", Types.ValueType.UNKNOWN, Types.DataType.UNKNOWN);
+
+ String[] parts = InstructionUtils.getInstructionPartsWithValueType(str);
+ String opcode = parts[0];
+
+ if( !opcode.equalsIgnoreCase("cov") ) {
+ throw new DMLRuntimeException("CovarianceCPInstruction.parseInstruction():: Unknown opcode " + opcode);
+ }
+
+ COVOperator cov = new COVOperator(COV.getCOMFnObject());
+ if ( parts.length == 4 ) {
+ parseBinaryInstruction(str, in1, in2, out);
+ return new CovarianceFEDInstruction(cov, in1, in2, out, opcode, str);
+ } else if ( parts.length == 5 ) {
+ in3 = new CPOperand("", Types.ValueType.UNKNOWN, Types.DataType.UNKNOWN);
+ parseBinaryInstruction(str, in1, in2, in3, out);
+ return new CovarianceFEDInstruction(cov, in1, in2, in3, out, opcode, str);
+ }
+ else {
+ throw new DMLRuntimeException("Invalid number of arguments in Instruction: " + str);
+ }
+ }
+
+ @Override
+ public void processInstruction(ExecutionContext ec) {
+ MatrixObject mo1 = ec.getMatrixObject(input1);
+ MatrixObject mo2 = ec.getMatrixObject(input2);
+ COVOperator cop = ((COVOperator)_optr);
+
+ if(mo1.isFederated() && mo2.isFederated() && !mo1.getFedMapping().isAligned(mo2.getFedMapping(), false))
+ throw new DMLRuntimeException("Not supported matrix-matrix binary operation: covariance.");
+
+ FederatedRequest fr2 = null;
+ if(mo1.isFederated() && mo2.isFederated() && mo1.getFedMapping().isAligned(mo2.getFedMapping(), false)) {
+ fr2 = FederationUtils.callInstruction(instString, output, new CPOperand[]{input1, input2},
+ new long[]{mo1.getFedMapping().getID(), mo2.getFedMapping().getID()});
+ Future<FederatedResponse>[] tmp = mo1.getFedMapping().execute(getTID(), true, fr2);
+
+ //means
+ Future<FederatedResponse>[] meanTmp1 = processMean(mo1, 0);
+ Future<FederatedResponse>[] meanTmp2 = processMean(mo2, 1);
+
+ double res = aggCov(tmp, meanTmp1, meanTmp2, mo1.getFedMapping());
+ ec.setVariable(output.getName(), new DoubleObject(res));
+ }
+ else {
+ MatrixBlock mb;
+ MatrixObject mo;
+ if(!mo1.isFederated() && mo2.isFederated()) {
+ mo = mo2;
+ mb = ec.getMatrixInput(input1.getName());
+ }
+ else {
+ mo = mo1;
+ mb = ec.getMatrixInput(input2.getName());
+ }
+
+ FederationMap fedMapping = mo.getFedMapping();
+ List<CM_COV_Object> globalCmobj = new ArrayList<>();
+ long varID = FederationUtils.getNextFedDataID();
+
+ fedMapping.mapParallel(varID, (range, data) -> {
Review comment:
map parallel, using a function on the right side of the lambda would also be cool here.
##########
File path: src/test/scripts/functions/federated/FederatedCovarianceTest.dml
##########
@@ -0,0 +1,46 @@
+#-------------------------------------------------------------
+#
+# 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.
+#
+#-------------------------------------------------------------
+
+if ($rP) {
+ A = federated(addresses=list($in_X1, $in_X2, $in_X3, $in_X4),
+ ranges=list(list(0, 0), list($rows/4, $cols), list($rows/4, 0), list(2*$rows/4, $cols),
+ list(2*$rows/4, 0), list(3*$rows/4, $cols), list(3*$rows/4, 0), list($rows, $cols)));
+ /*
+ B = federated(addresses=list($in_Y1, $in_Y2, $in_Y3, $in_Y4),
+ ranges=list(list(0, 0), list($rows/4, $cols), list($rows/4, 0), list(2*$rows/4, $cols),
+ list(2*$rows/4, 0), list(3*$rows/4, $cols), list(3*$rows/4, 0), list($rows, $cols)));
+ */
Review comment:
Remove out commented lines
##########
File path: src/test/scripts/functions/federated/FederatedCovarianceTestReference.dml
##########
@@ -0,0 +1,28 @@
+#-------------------------------------------------------------
+#
+# 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.
+#
+#-------------------------------------------------------------
+
+if($6) { A = rbind(read($1), read($2), read($3), read($4));
+ B = read($5);}
+else { A = cbind(read($1), read($2), read($3), read($4));
+ B = read($5);}
Review comment:
are you using both the if and else in this statement? if not delete it, or make sure you are.
##########
File path: src/test/java/org/apache/sysds/test/functions/federated/primitives/FederatedCovarianceTest.java
##########
@@ -0,0 +1,166 @@
+/*
+ * 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.federated.primitives;
+
+import java.util.Arrays;
+import java.util.Collection;
+
+import org.apache.sysds.api.DMLScript;
+import org.apache.sysds.common.Types.ExecMode;
+import org.apache.sysds.runtime.meta.MatrixCharacteristics;
+import org.apache.sysds.runtime.util.HDFSTool;
+import org.apache.sysds.test.AutomatedTestBase;
+import org.apache.sysds.test.TestConfiguration;
+import org.apache.sysds.test.TestUtils;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(value = Parameterized.class)
+@net.jcip.annotations.NotThreadSafe
+public class FederatedCovarianceTest extends AutomatedTestBase {
+
+ private final static String TEST_NAME = "FederatedCovarianceTest";
+
+ private final static String TEST_DIR = "functions/federated/";
+ private static final String TEST_CLASS_DIR = TEST_DIR + FederatedCovarianceTest.class.getSimpleName() + "/";
+
+ private final static int blocksize = 1024;
+ @Parameterized.Parameter()
+ public int rows;
+ @Parameterized.Parameter(1)
+ public int cols;
+
+ @Parameterized.Parameter(2)
+ public boolean rowPartitioned;
+
+ @Parameterized.Parameters
+ public static Collection<Object[]> data() {
+ return Arrays.asList(new Object[][] {
+ {20, 1, true},
Review comment:
while testing add a few more cases. then when you find it okay, reduce it to a few.
##########
File path: src/test/java/org/apache/sysds/test/functions/federated/primitives/FederatedCovarianceTest.java
##########
@@ -0,0 +1,166 @@
+/*
+ * 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.federated.primitives;
+
+import java.util.Arrays;
+import java.util.Collection;
+
+import org.apache.sysds.api.DMLScript;
+import org.apache.sysds.common.Types.ExecMode;
+import org.apache.sysds.runtime.meta.MatrixCharacteristics;
+import org.apache.sysds.runtime.util.HDFSTool;
+import org.apache.sysds.test.AutomatedTestBase;
+import org.apache.sysds.test.TestConfiguration;
+import org.apache.sysds.test.TestUtils;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(value = Parameterized.class)
+@net.jcip.annotations.NotThreadSafe
+public class FederatedCovarianceTest extends AutomatedTestBase {
+
+ private final static String TEST_NAME = "FederatedCovarianceTest";
+
+ private final static String TEST_DIR = "functions/federated/";
+ private static final String TEST_CLASS_DIR = TEST_DIR + FederatedCovarianceTest.class.getSimpleName() + "/";
+
+ private final static int blocksize = 1024;
+ @Parameterized.Parameter()
Review comment:
not needed paranthesis
##########
File path: src/test/java/org/apache/sysds/test/functions/federated/primitives/FederatedCovarianceTest.java
##########
@@ -0,0 +1,166 @@
+/*
+ * 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.federated.primitives;
+
+import java.util.Arrays;
+import java.util.Collection;
+
+import org.apache.sysds.api.DMLScript;
+import org.apache.sysds.common.Types.ExecMode;
+import org.apache.sysds.runtime.meta.MatrixCharacteristics;
+import org.apache.sysds.runtime.util.HDFSTool;
+import org.apache.sysds.test.AutomatedTestBase;
+import org.apache.sysds.test.TestConfiguration;
+import org.apache.sysds.test.TestUtils;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(value = Parameterized.class)
+@net.jcip.annotations.NotThreadSafe
+public class FederatedCovarianceTest extends AutomatedTestBase {
+
+ private final static String TEST_NAME = "FederatedCovarianceTest";
+
+ private final static String TEST_DIR = "functions/federated/";
+ private static final String TEST_CLASS_DIR = TEST_DIR + FederatedCovarianceTest.class.getSimpleName() + "/";
+
+ private final static int blocksize = 1024;
+ @Parameterized.Parameter()
+ public int rows;
+ @Parameterized.Parameter(1)
+ public int cols;
+
+ @Parameterized.Parameter(2)
+ public boolean rowPartitioned;
+
+ @Parameterized.Parameters
+ public static Collection<Object[]> data() {
+ return Arrays.asList(new Object[][] {
+ {20, 1, true},
+ });
+ }
+
+ @Override
+ public void setUp() {
+ TestUtils.clearAssertionInformation();
+ addTestConfiguration(TEST_NAME, new TestConfiguration(TEST_CLASS_DIR, TEST_NAME, new String[] {"S.scalar"}));
+ }
+
+ @Test
+ public void testCovCP() { runCovTest(ExecMode.SINGLE_NODE); }
+
+ private void runCovTest(ExecMode execMode) {
+ boolean sparkConfigOld = DMLScript.USE_LOCAL_SPARK_CONFIG;
+ ExecMode platformOld = rtplatform;
+
+ if(rtplatform == ExecMode.SPARK)
+ DMLScript.USE_LOCAL_SPARK_CONFIG = true;
+
+ getAndLoadTestConfiguration(TEST_NAME);
+ String HOME = SCRIPT_DIR + TEST_DIR;
+
+ // write input matrices
+ int r = rows;
+ int c = cols / 4;
+ if(rowPartitioned) {
+ r = rows / 4;
+ c = cols;
+ }
+
+
+ // empty script name because we don't execute any script, just start the worker
+ fullDMLScriptName = "";
+
+
+ double[][] X1 = getRandomMatrix(r, c, 1, 5, 1, 3);
+ double[][] X2 = getRandomMatrix(r, c, 1, 5, 1, 7);
+ double[][] X3 = getRandomMatrix(r, c, 1, 5, 1, 8);
+ double[][] X4 = getRandomMatrix(r, c, 1, 5, 1, 9);
+
+ double[][] Y = getRandomMatrix(rows, c, 1, 5, 1, 3);
+
+ // double[][] Y1 = getRandomMatrix(r, c, 1, 5, 1, 3);
+ // double[][] Y2 = getRandomMatrix(r, c, 1, 5, 1, 7);
+ // double[][] Y3 = getRandomMatrix(r, c, 1, 5, 1, 8);
+ // double[][] Y4 = getRandomMatrix(r, c, 1, 5, 1, 9);
Review comment:
remove out commented lines
##########
File path: src/test/java/org/apache/sysds/test/functions/federated/primitives/FederatedCovarianceTest.java
##########
@@ -0,0 +1,166 @@
+/*
+ * 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.federated.primitives;
+
+import java.util.Arrays;
+import java.util.Collection;
+
+import org.apache.sysds.api.DMLScript;
+import org.apache.sysds.common.Types.ExecMode;
+import org.apache.sysds.runtime.meta.MatrixCharacteristics;
+import org.apache.sysds.runtime.util.HDFSTool;
+import org.apache.sysds.test.AutomatedTestBase;
+import org.apache.sysds.test.TestConfiguration;
+import org.apache.sysds.test.TestUtils;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(value = Parameterized.class)
+@net.jcip.annotations.NotThreadSafe
+public class FederatedCovarianceTest extends AutomatedTestBase {
+
+ private final static String TEST_NAME = "FederatedCovarianceTest";
+
+ private final static String TEST_DIR = "functions/federated/";
+ private static final String TEST_CLASS_DIR = TEST_DIR + FederatedCovarianceTest.class.getSimpleName() + "/";
+
+ private final static int blocksize = 1024;
+ @Parameterized.Parameter()
+ public int rows;
+ @Parameterized.Parameter(1)
+ public int cols;
+
+ @Parameterized.Parameter(2)
+ public boolean rowPartitioned;
+
+ @Parameterized.Parameters
+ public static Collection<Object[]> data() {
+ return Arrays.asList(new Object[][] {
+ {20, 1, true},
+ });
+ }
+
+ @Override
+ public void setUp() {
+ TestUtils.clearAssertionInformation();
+ addTestConfiguration(TEST_NAME, new TestConfiguration(TEST_CLASS_DIR, TEST_NAME, new String[] {"S.scalar"}));
+ }
+
+ @Test
+ public void testCovCP() { runCovTest(ExecMode.SINGLE_NODE); }
+
+ private void runCovTest(ExecMode execMode) {
+ boolean sparkConfigOld = DMLScript.USE_LOCAL_SPARK_CONFIG;
+ ExecMode platformOld = rtplatform;
+
+ if(rtplatform == ExecMode.SPARK)
+ DMLScript.USE_LOCAL_SPARK_CONFIG = true;
+
+ getAndLoadTestConfiguration(TEST_NAME);
+ String HOME = SCRIPT_DIR + TEST_DIR;
+
+ // write input matrices
+ int r = rows;
+ int c = cols / 4;
+ if(rowPartitioned) {
+ r = rows / 4;
+ c = cols;
+ }
+
+
Review comment:
many new lines again
##########
File path: src/test/java/org/apache/sysds/test/functions/federated/primitives/FederatedCovarianceTest.java
##########
@@ -0,0 +1,166 @@
+/*
+ * 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.federated.primitives;
+
+import java.util.Arrays;
+import java.util.Collection;
+
+import org.apache.sysds.api.DMLScript;
+import org.apache.sysds.common.Types.ExecMode;
+import org.apache.sysds.runtime.meta.MatrixCharacteristics;
+import org.apache.sysds.runtime.util.HDFSTool;
+import org.apache.sysds.test.AutomatedTestBase;
+import org.apache.sysds.test.TestConfiguration;
+import org.apache.sysds.test.TestUtils;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(value = Parameterized.class)
+@net.jcip.annotations.NotThreadSafe
+public class FederatedCovarianceTest extends AutomatedTestBase {
+
+ private final static String TEST_NAME = "FederatedCovarianceTest";
+
Review comment:
Decide when to have a new line, and when not to in these variables. (Be consistent)
----------------------------------------------------------------
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
[GitHub] [systemds] Baunsgaard closed pull request #1150: [SYSTEMDS-2766] Federated covariance
Posted by GitBox <gi...@apache.org>.
Baunsgaard closed pull request #1150:
URL: https://github.com/apache/systemds/pull/1150
----------------------------------------------------------------
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