You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@systemds.apache.org by "e-strauss (via GitHub)" <gi...@apache.org> on 2024/09/11 16:09:46 UTC
[PR] [SYSTEMDS-????] rewrite nary elementwise mult [systemds]
e-strauss opened a new pull request, #2105:
URL: https://github.com/apache/systemds/pull/2105
This patch adds the nary operator for elementwise multiply
--
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
Re: [PR] [SYSTEMDS-????] rewrite nary elementwise mult [systemds]
Posted by "mboehm7 (via GitHub)" <gi...@apache.org>.
mboehm7 commented on PR #2105:
URL: https://github.com/apache/systemds/pull/2105#issuecomment-2366721771
LGTM - thanks for the patch @e-strauss and the holistic cleanup. During the merge I only replaced the unrolled loop in `processMultRowSparse` with a new `vectMultiplyInPlace` in `LibMatrixMult`.
--
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
Re: [PR] [SYSTEMDS-????] rewrite nary elementwise mult [systemds]
Posted by "e-strauss (via GitHub)" <gi...@apache.org>.
e-strauss commented on code in PR #2105:
URL: https://github.com/apache/systemds/pull/2105#discussion_r1755120908
##########
src/main/java/org/apache/sysds/runtime/matrix/data/MatrixBlock.java:
##########
@@ -3891,6 +3907,101 @@ private static void processAddRow(MatrixBlock[] inputs, double[] c, int cix, int
}
}
}
+
+ private static void processMultRowOld(MatrixBlock[] inputs, double[] c, int cix, int n, int i) {
Review Comment:
this function was just for testing and can be removed
--
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
Re: [PR] [SYSTEMDS-????] rewrite nary elementwise mult [systemds]
Posted by "e-strauss (via GitHub)" <gi...@apache.org>.
e-strauss commented on code in PR #2105:
URL: https://github.com/apache/systemds/pull/2105#discussion_r1755120908
##########
src/main/java/org/apache/sysds/runtime/matrix/data/MatrixBlock.java:
##########
@@ -3891,6 +3907,101 @@ private static void processAddRow(MatrixBlock[] inputs, double[] c, int cix, int
}
}
}
+
+ private static void processMultRowOld(MatrixBlock[] inputs, double[] c, int cix, int n, int i) {
Review Comment:
this function was just for testing and can be removed
--
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
Re: [PR] [SYSTEMDS-????] rewrite nary elementwise mult [systemds]
Posted by "e-strauss (via GitHub)" <gi...@apache.org>.
e-strauss commented on code in PR #2105:
URL: https://github.com/apache/systemds/pull/2105#discussion_r1758740300
##########
src/test/java/org/apache/sysds/test/functions/ternary/TernaryAggregateTest.java:
##########
@@ -41,10 +41,11 @@ public class TernaryAggregateTest extends AutomatedTestBase
{
private final static String TEST_NAME1 = "TernaryAggregateRC";
private final static String TEST_NAME2 = "TernaryAggregateC";
+ private final static String TEST_NAME3 = "TernaryAggregatePow";
private final static String TEST_DIR = "functions/ternary/";
private final static String TEST_CLASS_DIR = TEST_DIR + TernaryAggregateTest.class.getSimpleName() + "/";
- private final static double eps = 1e-8;
+ private final static double eps = 1e-7;
Review Comment:
had to change to 1e-7, because test failing locally on previous commits were the github tests passed (might be caused by local R version / ARM CPU ?)
--
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
Re: [PR] [SYSTEMDS-????] rewrite nary elementwise mult [systemds]
Posted by "e-strauss (via GitHub)" <gi...@apache.org>.
e-strauss commented on PR #2105:
URL: https://github.com/apache/systemds/pull/2105#issuecomment-2344151139
there seems to be an issue with precision, I will check that
--
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
Re: [PR] [SYSTEMDS-????] rewrite nary elementwise mult [systemds]
Posted by "mboehm7 (via GitHub)" <gi...@apache.org>.
mboehm7 closed pull request #2105: [SYSTEMDS-????] rewrite nary elementwise mult
URL: https://github.com/apache/systemds/pull/2105
--
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
Re: [PR] [SYSTEMDS-????] rewrite nary elementwise mult [systemds]
Posted by "e-strauss (via GitHub)" <gi...@apache.org>.
e-strauss commented on code in PR #2105:
URL: https://github.com/apache/systemds/pull/2105#discussion_r1758740300
##########
src/test/java/org/apache/sysds/test/functions/ternary/TernaryAggregateTest.java:
##########
@@ -41,10 +41,11 @@ public class TernaryAggregateTest extends AutomatedTestBase
{
private final static String TEST_NAME1 = "TernaryAggregateRC";
private final static String TEST_NAME2 = "TernaryAggregateC";
+ private final static String TEST_NAME3 = "TernaryAggregatePow";
private final static String TEST_DIR = "functions/ternary/";
private final static String TEST_CLASS_DIR = TEST_DIR + TernaryAggregateTest.class.getSimpleName() + "/";
- private final static double eps = 1e-8;
+ private final static double eps = 1e-7;
Review Comment:
had to change 1e-7, because test failing locally on previous commits were the github tests passed (might be caused by local R version / ARM CPU ?)
--
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
Re: [PR] [SYSTEMDS-????] rewrite nary elementwise mult [systemds]
Posted by "e-strauss (via GitHub)" <gi...@apache.org>.
e-strauss commented on code in PR #2105:
URL: https://github.com/apache/systemds/pull/2105#discussion_r1758742277
##########
src/test/java/org/apache/sysds/test/functions/ternary/TernaryAggregateTest.java:
##########
@@ -180,6 +182,36 @@ public void testTernaryAggregateCDenseMatrixCPNoRewrite() {
public void testTernaryAggregateCSparseMatrixCPNoRewrite() {
runTernaryAggregateTest(TEST_NAME2, true, false, false, ExecType.CP);
}
+
Review Comment:
added a new test cases for rewrite colsum(A^3) to improve code coverage
--
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
Re: [PR] [SYSTEMDS-????] rewrite nary elementwise mult [systemds]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2105:
URL: https://github.com/apache/systemds/pull/2105#issuecomment-2346574428
## [Codecov](https://app.codecov.io/gh/apache/systemds/pull/2105?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `88.48485%` with `19 lines` in your changes missing coverage. Please review.
> Project coverage is 70.65%. Comparing base [(`95cfb76`)](https://app.codecov.io/gh/apache/systemds/commit/95cfb76fee57e92d20c94b26b445d91819dfc5ee?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`b74e76a`)](https://app.codecov.io/gh/apache/systemds/commit/b74e76ac70efa5b2e55bedf7dab151d5aba16173?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
| [Files with missing lines](https://app.codecov.io/gh/apache/systemds/pull/2105?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...rc/main/java/org/apache/sysds/hops/AggUnaryOp.java](https://app.codecov.io/gh/apache/systemds/pull/2105?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fapache%2Fsysds%2Fhops%2FAggUnaryOp.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3N5c2RzL2hvcHMvQWdnVW5hcnlPcC5qYXZh) | 81.66% | [3 Missing and 8 partials :warning: ](https://app.codecov.io/gh/apache/systemds/pull/2105?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../apache/sysds/runtime/matrix/data/MatrixBlock.java](https://app.codecov.io/gh/apache/systemds/pull/2105?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fapache%2Fsysds%2Fruntime%2Fmatrix%2Fdata%2FMatrixBlock.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3N5c2RzL3J1bnRpbWUvbWF0cml4L2RhdGEvTWF0cml4QmxvY2suamF2YQ==) | 96.20% | [1 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/systemds/pull/2105?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...org/apache/sysds/hops/rewrite/HopRewriteUtils.java](https://app.codecov.io/gh/apache/systemds/pull/2105?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fapache%2Fsysds%2Fhops%2Frewrite%2FHopRewriteUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3N5c2RzL2hvcHMvcmV3cml0ZS9Ib3BSZXdyaXRlVXRpbHMuamF2YQ==) | 66.66% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/systemds/pull/2105?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...nstructions/cp/MatrixBuiltinNaryCPInstruction.java](https://app.codecov.io/gh/apache/systemds/pull/2105?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fapache%2Fsysds%2Fruntime%2Finstructions%2Fcp%2FMatrixBuiltinNaryCPInstruction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3N5c2RzL3J1bnRpbWUvaW5zdHJ1Y3Rpb25zL2NwL01hdHJpeEJ1aWx0aW5OYXJ5Q1BJbnN0cnVjdGlvbi5qYXZh) | 0.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/systemds/pull/2105?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...e/instructions/spark/BuiltinNarySPInstruction.java](https://app.codecov.io/gh/apache/systemds/pull/2105?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fapache%2Fsysds%2Fruntime%2Finstructions%2Fspark%2FBuiltinNarySPInstruction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3N5c2RzL3J1bnRpbWUvaW5zdHJ1Y3Rpb25zL3NwYXJrL0J1aWx0aW5OYXJ5U1BJbnN0cnVjdGlvbi5qYXZh) | 83.33% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/systemds/pull/2105?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...e/sysds/runtime/lineage/LineageRecomputeUtils.java](https://app.codecov.io/gh/apache/systemds/pull/2105?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fapache%2Fsysds%2Fruntime%2Flineage%2FLineageRecomputeUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3N5c2RzL3J1bnRpbWUvbGluZWFnZS9MaW5lYWdlUmVjb21wdXRlVXRpbHMuamF2YQ==) | 50.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/systemds/pull/2105?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2105 +/- ##
============================================
- Coverage 70.66% 70.65% -0.02%
- Complexity 42231 42266 +35
============================================
Files 1441 1441
Lines 162417 162506 +89
Branches 31654 31684 +30
============================================
+ Hits 114777 114823 +46
- Misses 38660 38695 +35
- Partials 8980 8988 +8
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/systemds/pull/2105?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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
Re: [PR] [SYSTEMDS-????] rewrite nary elementwise mult [systemds]
Posted by "e-strauss (via GitHub)" <gi...@apache.org>.
e-strauss commented on PR #2105:
URL: https://github.com/apache/systemds/pull/2105#issuecomment-2367470898
thanks, yes makes sense
--
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