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 2022/04/19 23:36:22 UTC
[GitHub] [systemds] OlgaOvcharenko opened a new pull request, #1590: [MINOR] Fix federated rmempty
OlgaOvcharenko opened a new pull request, #1590:
URL: https://github.com/apache/systemds/pull/1590
This PR fixes tests and one line in federated rmempty instruction for sparse matrices.
--
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] Baunsgaard commented on a diff in pull request #1590: [MINOR] Fix federated rmempty
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on code in PR #1590:
URL: https://github.com/apache/systemds/pull/1590#discussion_r853828775
##########
src/main/java/org/apache/sysds/runtime/instructions/fed/ParameterizedBuiltinFEDInstruction.java:
##########
@@ -906,8 +906,8 @@ public GetMatrixCharacteristics(long varID) {
@Override
public FederatedResponse execute(ExecutionContext ec, Data... data) {
MatrixBlock mb = ((MatrixObject) data[0]).acquireReadAndRelease();
- int r = mb.getDenseBlockValues() != null ? mb.getNumRows() : 0;
- int c = mb.getDenseBlockValues() != null ? mb.getNumColumns() : 0;
+ int r = mb.getDenseBlockValues() != null || mb.isInSparseFormat() ? mb.getNumRows() : 0;
+ int c = mb.getDenseBlockValues() != null || mb.isInSparseFormat() ? mb.getNumColumns() : 0;
Review Comment:
Cool, thanks for the fix.
But i think we can improve it to work with any matrix type, since you could use mb.isEmpty()
also, minor thing, in this code you use two calls, and you could make do with one, if you split the code in if else.
```
if(mb.isEmpty())
return new .... SUCCESS, new int[] {0,0});
return new ... SUCCESS, new int[] {mb.getNumRows(), mb.getNumColumns()};
```
Then everything works with compressed matrices as well :)
--
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] Baunsgaard commented on pull request #1590: [MINOR] Fix federated rmempty
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on PR #1590:
URL: https://github.com/apache/systemds/pull/1590#issuecomment-1103578041
LGTM will merge shortly with the commented change.
--
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] Baunsgaard closed pull request #1590: [MINOR] Fix federated rmempty
Posted by GitBox <gi...@apache.org>.
Baunsgaard closed pull request #1590: [MINOR] Fix federated rmempty
URL: https://github.com/apache/systemds/pull/1590
--
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] Baunsgaard commented on a diff in pull request #1590: [MINOR] Fix federated rmempty
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on code in PR #1590:
URL: https://github.com/apache/systemds/pull/1590#discussion_r853828775
##########
src/main/java/org/apache/sysds/runtime/instructions/fed/ParameterizedBuiltinFEDInstruction.java:
##########
@@ -906,8 +906,8 @@ public GetMatrixCharacteristics(long varID) {
@Override
public FederatedResponse execute(ExecutionContext ec, Data... data) {
MatrixBlock mb = ((MatrixObject) data[0]).acquireReadAndRelease();
- int r = mb.getDenseBlockValues() != null ? mb.getNumRows() : 0;
- int c = mb.getDenseBlockValues() != null ? mb.getNumColumns() : 0;
+ int r = mb.getDenseBlockValues() != null || mb.isInSparseFormat() ? mb.getNumRows() : 0;
+ int c = mb.getDenseBlockValues() != null || mb.isInSparseFormat() ? mb.getNumColumns() : 0;
Review Comment:
Cool, thanks for the fix.
But i think we can improve it to work with any matrix type, since you could use mb.isEmpty()
also, minor thing, in this code you use two calls, and you could make do with one, if you split the code in if else.
```
if(mb.isEmpty())
return new .... SUCESS, new int[] {0,0});
return new ... SUCESS, new int[] {mb.getNumRows(), mb.getNumColumns()};
```
Then everything works with compressed matrices as well :)
--
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