Baunsgaard commented on a change in pull request #1112:
URL: https://github.com/apache/systemds/pull/1112#discussion_r546675811
##########
File path:
src/main/java/org/apache/sysds/runtime/instructions/fed/ParameterizedBuiltinFEDInstruction.java
##########
@@ -145,22 +149,115 @@ else if(opcode.equalsIgnoreCase("transformapply"))
throw new DMLRuntimeException("Unknown opcode : " +
opcode);
}
}
+ private void triangle(ExecutionContext ec, String opcode) {
Review comment:
add space between methods.
##########
File path:
src/main/java/org/apache/sysds/runtime/instructions/fed/ParameterizedBuiltinFEDInstruction.java
##########
@@ -145,22 +149,115 @@ else if(opcode.equalsIgnoreCase("transformapply"))
throw new DMLRuntimeException("Unknown opcode : " +
opcode);
}
}
+ private void triangle(ExecutionContext ec, String opcode) {
+ boolean lower = opcode.equals("lowertri");
+ boolean diag = Boolean.parseBoolean(params.get("diag"));
+ boolean values = Boolean.parseBoolean(params.get("values"));
+
+ MatrixObject mo = (MatrixObject) getTarget(ec);
+
+ FederationMap fedMap = mo.getFedMapping();
+ boolean rowFed = mo.isFederated(FederationMap.FType.ROW);
+
+ long varID = FederationUtils.getNextFedDataID();
+ FederationMap diagFedMap;
+
+ diagFedMap = fedMap.mapParallel(varID, (range, data) -> {
+ try {
+ FederatedResponse response =
data.executeFederatedOperation(new FederatedRequest(
+ FederatedRequest.RequestType.EXEC_UDF,
-1,
+ new
ParameterizedBuiltinFEDInstruction.Tri(data.getVarID(), varID,
+ rowFed ? (new int[]
{range.getBeginDimsInt()[0], range.getEndDimsInt()[0]}) :
+ new int[]
{range.getBeginDimsInt()[1], range.getEndDimsInt()[1]},
+ rowFed, lower, diag,
values))).get();
+ if(!response.isSuccessful())
+ response.throwExceptionFromResponse();
+ return null;
+ }
+ catch(Exception e) {
+ throw new DMLRuntimeException(e);
+ }
+ });
+ MatrixObject out = ec.getMatrixObject(output);
+ out.setFedMapping(diagFedMap);
+ }
+
+ private static class Tri extends FederatedUDF {
+ private static final long serialVersionUID =
6254009025304038215L;
+
+ private final long _outputID;
+ private final int[] _slice;
+ private final boolean _rowFed;
+ private final boolean _lower;
+ private final boolean _diag;
+ private final boolean _values;
+
+ private Tri(long input, long outputID, int[] slice, boolean
rowFed, boolean lower, boolean diag, boolean values) {
+ super(new long[] {input});
+ _outputID = outputID;
+ _slice = slice;
+ _rowFed = rowFed;
+ _lower = lower;
+ _diag = diag;
+ _values = values;
+ }
+
+ @Override
+ public FederatedResponse execute(ExecutionContext ec, Data...
data) {
+ MatrixBlock mb = ((MatrixObject)
data[0]).acquireReadAndRelease();
+ MatrixBlock soresBlock, addBlock;
+ MatrixBlock ret;
+
+ //slice
+ soresBlock = _rowFed ?
+ mb.slice(0, mb.getNumRows()-1, _slice[0],
_slice[1]-1, new MatrixBlock()) :
+ mb.slice(_slice[0], _slice[1]-1);
+
+ //triangle
+ MatrixBlock tri = soresBlock.extractTriangular(new
MatrixBlock(), _lower, _diag, _values);
+ if(_rowFed) {
+ ret = new MatrixBlock(mb.getNumRows(),
mb.getNumColumns(), 0.0);
+ ret.copy(0, ret.getNumRows()-1, _slice[0],
_slice[1]-1, tri, false);
+ if(_slice[1] <= mb.getNumColumns()-1 &&
!_lower) {
+ addBlock = mb.slice(0,
mb.getNumRows()-1, _slice[1], mb.getNumColumns()-1, new MatrixBlock());
Review comment:
the slice function is nice to use, but ultimately inefficient for the
job you are doing here.
because the slice hash a large unnecessary overhead.
I would suggest extracting the sparse or dense block and processing that
from the mb variable.
I would also suggest not to include this optimization right now in this PR,
but add a todo for later.
##########
File path:
src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederationMap.java
##########
@@ -384,6 +384,21 @@ private static void setThreadID(long tid,
FederatedRequest[]... frsets) {
Arrays.stream(frset).forEach(fr ->
fr.setTID(tid));
}
+ public void reverseFedMap() {
Review comment:
This assumes that all federated maps are always sorted in order. I think
that is not a constraint that we have.
Furthermore this would break if we have a federated mapping that is the
custom variant, with ranges all over the place.
I think maybe that you actually don't want to modify the mapping at all like
this, or make sure that you sort the ranges before attempting this.
----------------------------------------------------------------
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:
[email protected]
