You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@systemds.apache.org by "jessicapriebe (via GitHub)" <gi...@apache.org> on 2026/02/24 13:56:02 UTC
[PR] Add OOC cbind [systemds]
jessicapriebe opened a new pull request, #2437:
URL: https://github.com/apache/systemds/pull/2437
This adds OOC cbind.
--
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] Add OOC cbind [systemds]
Posted by "janniklinde (via GitHub)" <gi...@apache.org>.
janniklinde merged PR #2437:
URL: https://github.com/apache/systemds/pull/2437
--
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] Add OOC cbind [systemds]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2437:
URL: https://github.com/apache/systemds/pull/2437#issuecomment-4030532216
## [Codecov](https://app.codecov.io/gh/apache/systemds/pull/2437?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `82.35294%` with `24 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 71.80%. Comparing base ([`49893a8`](https://app.codecov.io/gh/apache/systemds/commit/49893a81526f141daa096886ada5bf18b704b92b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`9210d79`](https://app.codecov.io/gh/apache/systemds/commit/9210d798afcecbfe0ac4aa036ce7f79a41e174e2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 8 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/systemds/pull/2437?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...runtime/instructions/ooc/AppendOOCInstruction.java](https://app.codecov.io/gh/apache/systemds/pull/2437?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fapache%2Fsysds%2Fruntime%2Finstructions%2Fooc%2FAppendOOCInstruction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3N5c2RzL3J1bnRpbWUvaW5zdHJ1Y3Rpb25zL29vYy9BcHBlbmRPT0NJbnN0cnVjdGlvbi5qYXZh) | 93.26% | [5 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/systemds/pull/2437?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...untime/instructions/ooc/SubscribableTaskQueue.java](https://app.codecov.io/gh/apache/systemds/pull/2437?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fapache%2Fsysds%2Fruntime%2Finstructions%2Fooc%2FSubscribableTaskQueue.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3N5c2RzL3J1bnRpbWUvaW5zdHJ1Y3Rpb25zL29vYy9TdWJzY3JpYmFibGVUYXNrUXVldWUuamF2YQ==) | 25.00% | [2 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/systemds/pull/2437?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...org/apache/sysds/runtime/io/WriterBinaryBlock.java](https://app.codecov.io/gh/apache/systemds/pull/2437?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fapache%2Fsysds%2Fruntime%2Fio%2FWriterBinaryBlock.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3N5c2RzL3J1bnRpbWUvaW8vV3JpdGVyQmluYXJ5QmxvY2suamF2YQ==) | 0.00% | [4 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/systemds/pull/2437?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...va/org/apache/sysds/runtime/ooc/util/OOCUtils.java](https://app.codecov.io/gh/apache/systemds/pull/2437?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fapache%2Fsysds%2Fruntime%2Fooc%2Futil%2FOOCUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3N5c2RzL3J1bnRpbWUvb29jL3V0aWwvT09DVXRpbHMuamF2YQ==) | 20.00% | [2 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/systemds/pull/2437?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../sysds/runtime/instructions/ooc/CachingStream.java](https://app.codecov.io/gh/apache/systemds/pull/2437?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fapache%2Fsysds%2Fruntime%2Finstructions%2Fooc%2FCachingStream.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3N5c2RzL3J1bnRpbWUvaW5zdHJ1Y3Rpb25zL29vYy9DYWNoaW5nU3RyZWFtLmphdmE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/systemds/pull/2437?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...he/sysds/runtime/io/WriterBinaryBlockParallel.java](https://app.codecov.io/gh/apache/systemds/pull/2437?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fapache%2Fsysds%2Fruntime%2Fio%2FWriterBinaryBlockParallel.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3N5c2RzL3J1bnRpbWUvaW8vV3JpdGVyQmluYXJ5QmxvY2tQYXJhbGxlbC5qYXZh) | 75.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/systemds/pull/2437?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 #2437 +/- ##
============================================
+ Coverage 71.45% 71.80% +0.35%
- Complexity 47805 48261 +456
============================================
Files 1554 1555 +1
Lines 184605 185481 +876
Branches 36227 36401 +174
============================================
+ Hits 131903 133185 +1282
+ Misses 42449 41995 -454
- Partials 10253 10301 +48
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/systemds/pull/2437?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).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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] Add OOC cbind [systemds]
Posted by "janniklinde (via GitHub)" <gi...@apache.org>.
janniklinde commented on PR #2437:
URL: https://github.com/apache/systemds/pull/2437#issuecomment-4022009515
I think there may be an issue with parallelization of parameterized tests. Maybe try adding `@NotThreadSafe` to the test.
--
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] Add OOC cbind [systemds]
Posted by "janniklinde (via GitHub)" <gi...@apache.org>.
janniklinde commented on code in PR #2437:
URL: https://github.com/apache/systemds/pull/2437#discussion_r2884768622
##########
src/test/java/org/apache/sysds/test/functions/ooc/CBindTest.java:
##########
@@ -0,0 +1,140 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sysds.test.functions.ooc;
+
+import org.apache.sysds.common.Opcodes;
+import org.apache.sysds.common.Types;
+import org.apache.sysds.runtime.instructions.Instruction;
+import org.apache.sysds.runtime.io.MatrixWriter;
+import org.apache.sysds.runtime.io.MatrixWriterFactory;
+import org.apache.sysds.runtime.matrix.data.MatrixBlock;
+import org.apache.sysds.runtime.meta.MatrixCharacteristics;
+import org.apache.sysds.runtime.util.DataConverter;
+import org.apache.sysds.runtime.util.HDFSTool;
+import org.apache.sysds.test.AutomatedTestBase;
+import org.apache.sysds.test.TestConfiguration;
+import org.apache.sysds.test.TestUtils;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class CBindTest extends AutomatedTestBase {
+
+ private static final String TEST_NAME = "CBindTest";
+ private static final String TEST_DIR = "functions/ooc/";
+ private static final String TEST_CLASS_DIR = TEST_DIR + CBindTest.class.getSimpleName() + "/";
+
+ private final static double eps = 1e-8;
+ private static final String INPUT_NAME_1 = "A";
+ private static final String INPUT_NAME_2 = "B";
+ private static final String OUTPUT_NAME = "res";
+
+ @Override
+ public void setUp() {
+ TestUtils.clearAssertionInformation();
+ addTestConfiguration(TEST_NAME, new TestConfiguration(TEST_CLASS_DIR, TEST_NAME));
+ }
+
+ @Test
+ public void testCBindAppendBlock() { runCBindTest(1000, 1000, 1000, 1000);}
+
+ @Test
+ public void testCBindAppendBlockTwoLeftBlocks() {runCBindTest(1000, 2000, 1000, 1000);}
+
+ @Test
+ public void testCBindPartialFillSingleRightBlock() { runCBindTest(1000, 1100, 1000, 100);}
+
+ @Test
+ public void testCBindTotalFillSingleBlockEachSide() { runCBindTest(1000, 500, 1000, 500);}
+
+ @Test
+ public void testCBindTotalFillTwoRightBlocks() { runCBindTest(1000, 3500, 1000, 1500);}
+
+ @Test
+ public void testCBindPartialFillMultipleBlocksEachSide() { runCBindTest(1000, 3100, 1000, 3200);}
+
+ @Test
+ public void testCBindTotalFillMultipleBlocksEachSide() { runCBindTest(1000, 3500, 1000, 3500);}
+
+ @Test
+ public void testCBindOverflowSingleRightBlock() { runCBindTest(1000, 1600, 1000, 600);}
+
+ @Test
+ public void testCBindOverflowMultipleRightBlocks() { runCBindTest(1000, 1600, 1000, 2600);}
+
+ @Test
+ public void testCBindOverflowTotalFilledSingleRightBlock() {runCBindTest(1000, 1100, 1000, 1000);}
+
+ @Test
+ public void testCBindOverflowTotalFilledTwoRightBlocks() {runCBindTest(1000, 1100, 1000, 2000);}
+
+ @Test
+ public void testCBindMultipleRows() { runCBindTest(2500, 1500, 2500, 1500);}
+
+ @Test
+ public void testCBind() {runCBindTest(2300, 1655, 2300, 2542);}
Review Comment:
These edge cases are indeed good to have. Currently, OOC largely misses these tests for other operators as well (I will work on it to increase coverage). As I am aware of some bugs for different block sizes, it is good to have that additional parameter but not use different block sizes for now (like you did in the last commit).
--
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] Add OOC cbind [systemds]
Posted by "janniklinde (via GitHub)" <gi...@apache.org>.
janniklinde commented on code in PR #2437:
URL: https://github.com/apache/systemds/pull/2437#discussion_r2884768622
##########
src/test/java/org/apache/sysds/test/functions/ooc/CBindTest.java:
##########
@@ -0,0 +1,140 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sysds.test.functions.ooc;
+
+import org.apache.sysds.common.Opcodes;
+import org.apache.sysds.common.Types;
+import org.apache.sysds.runtime.instructions.Instruction;
+import org.apache.sysds.runtime.io.MatrixWriter;
+import org.apache.sysds.runtime.io.MatrixWriterFactory;
+import org.apache.sysds.runtime.matrix.data.MatrixBlock;
+import org.apache.sysds.runtime.meta.MatrixCharacteristics;
+import org.apache.sysds.runtime.util.DataConverter;
+import org.apache.sysds.runtime.util.HDFSTool;
+import org.apache.sysds.test.AutomatedTestBase;
+import org.apache.sysds.test.TestConfiguration;
+import org.apache.sysds.test.TestUtils;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class CBindTest extends AutomatedTestBase {
+
+ private static final String TEST_NAME = "CBindTest";
+ private static final String TEST_DIR = "functions/ooc/";
+ private static final String TEST_CLASS_DIR = TEST_DIR + CBindTest.class.getSimpleName() + "/";
+
+ private final static double eps = 1e-8;
+ private static final String INPUT_NAME_1 = "A";
+ private static final String INPUT_NAME_2 = "B";
+ private static final String OUTPUT_NAME = "res";
+
+ @Override
+ public void setUp() {
+ TestUtils.clearAssertionInformation();
+ addTestConfiguration(TEST_NAME, new TestConfiguration(TEST_CLASS_DIR, TEST_NAME));
+ }
+
+ @Test
+ public void testCBindAppendBlock() { runCBindTest(1000, 1000, 1000, 1000);}
+
+ @Test
+ public void testCBindAppendBlockTwoLeftBlocks() {runCBindTest(1000, 2000, 1000, 1000);}
+
+ @Test
+ public void testCBindPartialFillSingleRightBlock() { runCBindTest(1000, 1100, 1000, 100);}
+
+ @Test
+ public void testCBindTotalFillSingleBlockEachSide() { runCBindTest(1000, 500, 1000, 500);}
+
+ @Test
+ public void testCBindTotalFillTwoRightBlocks() { runCBindTest(1000, 3500, 1000, 1500);}
+
+ @Test
+ public void testCBindPartialFillMultipleBlocksEachSide() { runCBindTest(1000, 3100, 1000, 3200);}
+
+ @Test
+ public void testCBindTotalFillMultipleBlocksEachSide() { runCBindTest(1000, 3500, 1000, 3500);}
+
+ @Test
+ public void testCBindOverflowSingleRightBlock() { runCBindTest(1000, 1600, 1000, 600);}
+
+ @Test
+ public void testCBindOverflowMultipleRightBlocks() { runCBindTest(1000, 1600, 1000, 2600);}
+
+ @Test
+ public void testCBindOverflowTotalFilledSingleRightBlock() {runCBindTest(1000, 1100, 1000, 1000);}
+
+ @Test
+ public void testCBindOverflowTotalFilledTwoRightBlocks() {runCBindTest(1000, 1100, 1000, 2000);}
+
+ @Test
+ public void testCBindMultipleRows() { runCBindTest(2500, 1500, 2500, 1500);}
+
+ @Test
+ public void testCBind() {runCBindTest(2300, 1655, 2300, 2542);}
Review Comment:
These edge cases are indeed good to have. Currently, OOC largely misses these tests for other operators as well (I will work on it to increase coverage). As I am aware of some bugs for different block sizes, I suggest leaving the block size constant for now (but keep the block size parameter like you did in the last commit).
--
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