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/06/14 07:16:17 UTC
[GitHub] [systemds] ywcb00 opened a new pull request #1314: [SYSTEMDS-2982] Federated instructions w/ multiple aligned matrices
ywcb00 opened a new pull request #1314:
URL: https://github.com/apache/systemds/pull/1314
Hi,
This PR adds support for federated **binary, aggregate ternary, aggregate binary, ctable, and mmchain** instructions with multiple **aligned** federated matrices.
I introduced a new enum _AType_ and the method _isAligned(AType)_ inside the FederationMap to simplify the alignment checks (to check for multiple possible partial alignments with a single call).
To avoid redundant code, I created a new static method _isFederated()_ in _SpoofFEDInstruction_, which is only a consolidation of the _isFederated()_ method of _SpoofCPInstruction_ and _SpoofSPInstruction_.
In addition, I changed several tests in order to also test multiple federated inputs.
Thanks for review :)
--
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] sebwrede commented on a change in pull request #1314: [SYSTEMDS-2982] Federated instructions w/ multiple aligned matrices
Posted by GitBox <gi...@apache.org>.
sebwrede commented on a change in pull request #1314:
URL: https://github.com/apache/systemds/pull/1314#discussion_r650943547
##########
File path: src/main/java/org/apache/sysds/runtime/instructions/cp/SpoofCPInstruction.java
##########
@@ -138,34 +136,6 @@ public boolean isFederated(ExecutionContext ec) {
}
public boolean isFederated(ExecutionContext ec, FType type) {
Review comment:
It seems a bit counter-intuitive that a CP instruction has an `isFederated` method. Could we move this to `FedInstructionUtils.checkAndReplaceCP` which I believe is the only place where a call to `isFederated` is needed. Now that you cut the internals of the method to a single line this should be easy.
--
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 #1314: [SYSTEMDS-2982] Federated instructions w/ multiple aligned matrices
Posted by GitBox <gi...@apache.org>.
Baunsgaard closed pull request #1314:
URL: https://github.com/apache/systemds/pull/1314
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] ywcb00 commented on pull request #1314: [SYSTEMDS-2982] Federated instructions w/ multiple aligned matrices
Posted by GitBox <gi...@apache.org>.
ywcb00 commented on pull request #1314:
URL: https://github.com/apache/systemds/pull/1314#issuecomment-861546326
Thank you for the feedback @sebwrede :)
I changed the code according to your suggestions.
--
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] ywcb00 commented on a change in pull request #1314: [SYSTEMDS-2982] Federated instructions w/ multiple aligned matrices
Posted by GitBox <gi...@apache.org>.
ywcb00 commented on a change in pull request #1314:
URL: https://github.com/apache/systemds/pull/1314#discussion_r651794233
##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederationMap.java
##########
@@ -98,6 +98,29 @@ public boolean isType(FType t) {
}
}
+ // Alignment Check Type
+ public enum AType {
+ FULL,
+ ROW,
+ COL,
+ FULL_T,
+ ROW_T,
+ COL_T;
Review comment:
Yes, sure :)
--
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] sebwrede commented on a change in pull request #1314: [SYSTEMDS-2982] Federated instructions w/ multiple aligned matrices
Posted by GitBox <gi...@apache.org>.
sebwrede commented on a change in pull request #1314:
URL: https://github.com/apache/systemds/pull/1314#discussion_r650957474
##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederationMap.java
##########
@@ -98,6 +98,29 @@ public boolean isType(FType t) {
}
}
+ // Alignment Check Type
+ public enum AType {
+ FULL,
+ ROW,
+ COL,
+ FULL_T,
+ ROW_T,
+ COL_T;
Review comment:
Could you add a short description of each case in a comment next to the type (similar to the other enums in FederationMap).
--
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] sebwrede commented on a change in pull request #1314: [SYSTEMDS-2982] Federated instructions w/ multiple aligned matrices
Posted by GitBox <gi...@apache.org>.
sebwrede commented on a change in pull request #1314:
URL: https://github.com/apache/systemds/pull/1314#discussion_r650945942
##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederationMap.java
##########
@@ -98,6 +98,29 @@ public boolean isType(FType t) {
}
}
+ // Alignment Check Type
+ public enum AType {
Review comment:
Suggestion:
Could this be called "AlignType" or something else that is a bit more specific. I read "AType" as "a type" which is a bit too generic :smiley:
--
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] ywcb00 commented on a change in pull request #1314: [SYSTEMDS-2982] Federated instructions w/ multiple aligned matrices
Posted by GitBox <gi...@apache.org>.
ywcb00 commented on a change in pull request #1314:
URL: https://github.com/apache/systemds/pull/1314#discussion_r651788108
##########
File path: src/main/java/org/apache/sysds/runtime/instructions/cp/SpoofCPInstruction.java
##########
@@ -138,34 +136,6 @@ public boolean isFederated(ExecutionContext ec) {
}
public boolean isFederated(ExecutionContext ec, FType type) {
Review comment:
You're right :)
I changed it to directly call the static method _isFederated()_ from _SpoofFEDInstruction_ and introduced the getter method _getInputs()_ in _SpoofCPInstruction_ and _SpoofSPInstruction_ to get the inputs for that call.
--
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] ywcb00 commented on a change in pull request #1314: [SYSTEMDS-2982] Federated instructions w/ multiple aligned matrices
Posted by GitBox <gi...@apache.org>.
ywcb00 commented on a change in pull request #1314:
URL: https://github.com/apache/systemds/pull/1314#discussion_r651793398
##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederationMap.java
##########
@@ -98,6 +98,29 @@ public boolean isType(FType t) {
}
}
+ // Alignment Check Type
+ public enum AType {
Review comment:
It was so tempting to name it _AType_ alongside the other enum named _FType_ :rofl:
I changed the name to _AlignType_ now :+1:
--
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