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/08/11 13:48:17 UTC

[GitHub] [systemds] ywcb00 opened a new pull request #1362: [SYSTEMDS-3091] MatrixBlock Initialized with Zeros is Sparse

ywcb00 opened a new pull request #1362:
URL: https://github.com/apache/systemds/pull/1362


   Hi,
   This PR tries to remove the performance bottleneck inside the federated ctable instruction (caused by aggregating the sparse partial result matrices using a dense plus operator) by constructing a MatrixBlock as a sparse matrix if all the values are initialized to zero.
   
   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.

To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [systemds] mboehm7 commented on pull request #1362: [SYSTEMDS-3091] MatrixBlock Initialized with Zeros is Sparse

Posted by GitBox <gi...@apache.org>.
mboehm7 commented on pull request #1362:
URL: https://github.com/apache/systemds/pull/1362#issuecomment-898735466


   LGTM - great catch @ywcb00 and thanks for fixing this performance issue. For local CP and distributed Spark operations we actually decide based on estimated sparsity if we do a sparse hash aggregation or an accumulation into a dense matrix. Likely the federated ctable operations was implemented for correctness so far. In your experiments, however, please test both sparse ctable scenario (like creating permutation matrices) and dense scenarios (like confusion matrices with moderate number of classes). In any case, the revised change is much better, because the previous commit affected the defaults, partially without considering the value, and unintended sparse accumulation can be vastly slower due to binary search and potential shifting. But you're right reevaluated these defaults (e.g., with our revived perftest suite). This reminds me, if feel it's beneficial, please add your federated algorithm tests to this perftest so we can automatically run them before releases.


-- 
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] asfgit closed pull request #1362: [SYSTEMDS-3091] MatrixBlock Initialized with Zeros is Sparse

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1362:
URL: https://github.com/apache/systemds/pull/1362


   


-- 
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] mboehm7 commented on pull request #1362: [SYSTEMDS-3091] MatrixBlock Initialized with Zeros is Sparse

Posted by GitBox <gi...@apache.org>.
mboehm7 commented on pull request #1362:
URL: https://github.com/apache/systemds/pull/1362#issuecomment-897758533


   Since the implications of these changed defaults can be large, I would appreciate if we could address this ctable performance locally by passing the right parameters from outside `MatrixBlock`. Would that be possible or did you encounter any issues there?


-- 
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 #1362: [SYSTEMDS-3091] MatrixBlock Initialized with Zeros is Sparse

Posted by GitBox <gi...@apache.org>.
ywcb00 commented on pull request #1362:
URL: https://github.com/apache/systemds/pull/1362#issuecomment-898302871


   Yes, it is possible to address it locally.
   I've changed it accordingly :)
   
   However, I think we shouldn't forget the change of the MatrixBlock right away, because it accelerates the CTable Instruction by a factor of 100 in my experiments. Maybe it's the same with other instructions. :thinking: 
   


-- 
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