Baunsgaard commented on a change in pull request #1455:
URL: https://github.com/apache/systemds/pull/1455#discussion_r750381840
##########
File path: src/main/java/org/apache/sysds/runtime/matrix/data/FrameBlock.java
##########
@@ -2533,7 +2539,8 @@ private FrameBlock removeEmptyRows(MatrixBlock select,
boolean emptyReturn) {
isEmpty = isEmpty && (ArrayUtils.contains(new
double[]{0.0, Double.NaN}, UtilFunctions.objectToDoubleSafe(type,
colData.get(i))));
}
- if((!isEmpty && select == null) || (select != null &&
select.getValue(i, 0) == 1)) {
+ double selectValue = select != null ? !selectColVector
? select.getValue(i, 0) : select.getValue( 0, i) : 0.0;
+ if(selectValue == 1.0 || (!isEmpty && select == null)) {
Review comment:
This method is not really implemented efficiently.
I suggest the following:
1. time your tests to get a baseline time of the method
2. Avoid the clone on line 2536 (i know it is not your code but it should be
possible avoid.)
3. Materialize the select rows as a list of integers before the loop. If the
input was a sparse row vector this should be very simple and require no
allocation, otherwise allocate the list based on NNZ. (isolate this in a
separate function)
4. Change the for loop to only go through your new list of integers since
these represent the rows you want.
After this do the same for remove empty columns.
##########
File path:
src/test/java/org/apache/sysds/test/component/frame/FrameRemoveEmptyTest.java
##########
@@ -46,25 +46,25 @@ public void setUp() {
}
@Test
- public void testRemoveEmptyRowsDenseCP() {
+ public void testRemoveEmptyRowsCP() {
runTestRemoveEmpty(TEST_NAME1, "rows", Types.ExecType.CP,
false);
}
@Test
- public void testRemoveEmptyRowsSparseCP() {
+ public void testRemoveEmptyColsCP() {
runTestRemoveEmpty(TEST_NAME1, "cols", Types.ExecType.CP, true);
}
@Test
@Ignore
Review comment:
should these still be ignored?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
