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/09/02 14:45:22 UTC
[GitHub] [systemds] ywcb00 opened a new pull request #1381: [SYSTEMDS-3086] Federated WDivMM - Federated Output
ywcb00 opened a new pull request #1381:
URL: https://github.com/apache/systemds/pull/1381
Hi,
This PR adds the overwritten changes from the commit [5e3863842](https://github.com/apache/systemds/commit/5e386384296a781cf6d3adf1ffe52105e4356407) again.
It was overwritten by [6649bcd5b](https://github.com/apache/systemds/commit/6649bcd5bc8c32abae76aaf54315b48d79ff5397).
--
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 #1381: [SYSTEMDS-3086] Federated WDivMM - Federated Output
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1381:
URL: https://github.com/apache/systemds/pull/1381
--
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 #1381: [SYSTEMDS-3086] Federated WDivMM - Federated Output
Posted by GitBox <gi...@apache.org>.
ywcb00 commented on pull request #1381:
URL: https://github.com/apache/systemds/pull/1381#issuecomment-915902834
It wasn't overwritten on purpose.
The additional methods are already taken from the current test cases, but the tests do not verify that the output is kept federated.
We should definitely change the tests so that they fail if the output is not federated when it should be.
Since this is the case with several instructions, I will create a new jira task for this and add it with another PR.
Btw, do you know any better possibility to verify the federated output than performing an additional (unary) instruction on it (e.g., _fed_!=_) and verifying that this additional instruction is performed federated?
--
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] sebwrede commented on pull request #1381: [SYSTEMDS-3086] Federated WDivMM - Federated Output
Posted by GitBox <gi...@apache.org>.
sebwrede commented on pull request #1381:
URL: https://github.com/apache/systemds/pull/1381#issuecomment-915865124
Do you know if it was overwritten on purpose?
--
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 edited a comment on pull request #1381: [SYSTEMDS-3086] Federated WDivMM - Federated Output
Posted by GitBox <gi...@apache.org>.
ywcb00 edited a comment on pull request #1381:
URL: https://github.com/apache/systemds/pull/1381#issuecomment-915902834
It wasn't overwritten on purpose.
The additional methods are already taken from the current test cases, but the tests do not verify that the output is kept federated.
We should definitely change the tests so that they fail if the output is not federated when it should be.
Since this is the case with several instructions, I will create a new jira task for this and add it with another PR.
Btw, do you know any better possibility to verify that the output is federated than performing an additional (unary) instruction on it (e.g., _fed_!=_) and verifying that this additional instruction is performed federated?
--
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] sebwrede commented on pull request #1381: [SYSTEMDS-3086] Federated WDivMM - Federated Output
Posted by GitBox <gi...@apache.org>.
sebwrede commented on pull request #1381:
URL: https://github.com/apache/systemds/pull/1381#issuecomment-915867870
We can merge this again, but I think that if the change could be overwritten without any tests failing then it is a hint that we should add some tests that actually needs this additional processing case. This could be done in another PR or in the current PR.
--
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 #1381: [SYSTEMDS-3086] Federated WDivMM - Federated Output
Posted by GitBox <gi...@apache.org>.
mboehm7 commented on pull request #1381:
URL: https://github.com/apache/systemds/pull/1381#issuecomment-922368302
LGTM - thanks for fixing this @ywcb00. The overwrite was my fault, it was unintentional and happened during a massive merge of ~40 commits with many conflicts.
--
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] sebwrede commented on pull request #1381: [SYSTEMDS-3086] Federated WDivMM - Federated Output
Posted by GitBox <gi...@apache.org>.
sebwrede commented on pull request #1381:
URL: https://github.com/apache/systemds/pull/1381#issuecomment-915964606
Creating a new Jira task sounds like a good idea.
Computing an additional instruction and looking at the heavy-hitters is also the way I test federated outputs. You can see that in my current PR #1367.
--
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 edited a comment on pull request #1381: [SYSTEMDS-3086] Federated WDivMM - Federated Output
Posted by GitBox <gi...@apache.org>.
ywcb00 edited a comment on pull request #1381:
URL: https://github.com/apache/systemds/pull/1381#issuecomment-915902834
It wasn't overwritten on purpose.
The additional methods are already taken from the current test cases, but the tests do not verify that the output is kept federated.
We should definitely change the tests so that they fail if the output is not federated when it should be.
Since this is the case with several instructions, I will create a new jira task for this and add it with another PR.
Btw, do you know any better possibility to verify that the output is federated than performing an additional (unary) instruction on it (e.g., fed_!=) and verifying that this additional instruction is performed federated?
--
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