Baunsgaard commented on code in PR #1833:
URL: https://github.com/apache/systemds/pull/1833#discussion_r1205236799
##########
src/main/java/org/apache/sysds/runtime/compress/colgroup/dictionary/DeltaDictionary.java:
##########
@@ -20,26 +20,90 @@
package org.apache.sysds.runtime.compress.colgroup.dictionary;
import org.apache.commons.lang.NotImplementedException;
-import org.apache.sysds.runtime.functionobjects.Divide;
-import org.apache.sysds.runtime.functionobjects.Minus;
-import org.apache.sysds.runtime.functionobjects.Multiply;
-import org.apache.sysds.runtime.functionobjects.Plus;
+import org.apache.sysds.runtime.compress.colgroup.indexes.IColIndex;
+import org.apache.sysds.runtime.data.SparseBlock;
+import org.apache.sysds.runtime.functionobjects.*;
Review Comment:
we do not allow wildcard imports, instead import only the function objects
in use,
There should be a setting for your IDE to change this.
##########
src/main/java/org/apache/sysds/runtime/compress/colgroup/ColGroupDeltaDDC.java:
##########
@@ -19,11 +19,271 @@
package org.apache.sysds.runtime.compress.colgroup;
+
+import org.apache.commons.lang.NotImplementedException;
+import org.apache.sysds.runtime.compress.colgroup.dictionary.ADictionary;
+import org.apache.sysds.runtime.compress.colgroup.dictionary.DeltaDictionary;
+import org.apache.sysds.runtime.compress.colgroup.indexes.IColIndex;
+import org.apache.sysds.runtime.compress.colgroup.mapping.AMapToData;
+import org.apache.sysds.runtime.compress.colgroup.scheme.ICLAScheme;
+import org.apache.sysds.runtime.compress.cost.ComputationCostEstimator;
+import org.apache.sysds.runtime.data.DenseBlock;
+import org.apache.sysds.runtime.data.SparseBlock;
+import org.apache.sysds.runtime.functionobjects.Builtin;
+import org.apache.sysds.runtime.instructions.cp.CM_COV_Object;
+import org.apache.sysds.runtime.matrix.data.MatrixBlock;
+import org.apache.sysds.runtime.matrix.operators.BinaryOperator;
+import org.apache.sysds.runtime.matrix.operators.CMOperator;
+import org.apache.sysds.runtime.matrix.operators.ScalarOperator;
+import org.apache.sysds.runtime.matrix.operators.UnaryOperator;
+
/**
* Class to encapsulate information about a column group that is first delta
encoded then encoded with dense dictionary
* encoding (DeltaDDC).
*/
-public class ColGroupDeltaDDC { // extends ColGroupDDC
+public class ColGroupDeltaDDC extends AColGroupCompressed implements
AMapToDataGroup {
+
+ private static final long serialVersionUID = -1045556313148564147L;
+ private AMapToData _data;
+ private DeltaDictionary _dict;
+ //TODO Confirm if casting from ADictionary to DeltaDictionary is reasonable
+ private ColGroupDeltaDDC(IColIndex colIndexes, ADictionary dict,
AMapToData data) {
+ super(colIndexes);
+ _data = data;
+ _dict = (DeltaDictionary) dict;
+ }
+
+ //TODO Confirm if other parameters for constructors are required, such as
cachedCounts
+ public static AColGroup create(IColIndex colIndexes, ADictionary dict,
AMapToData data) {
+ if(data.getUnique() == 1)
+ return ColGroupConst.create(colIndexes, dict);
Review Comment:
Not valid. since const is not supporting delta encoding.
##########
src/main/java/org/apache/sysds/runtime/compress/colgroup/ColGroupDeltaDDC.java:
##########
@@ -19,11 +19,271 @@
package org.apache.sysds.runtime.compress.colgroup;
+
+import org.apache.commons.lang.NotImplementedException;
+import org.apache.sysds.runtime.compress.colgroup.dictionary.ADictionary;
+import org.apache.sysds.runtime.compress.colgroup.dictionary.DeltaDictionary;
+import org.apache.sysds.runtime.compress.colgroup.indexes.IColIndex;
+import org.apache.sysds.runtime.compress.colgroup.mapping.AMapToData;
+import org.apache.sysds.runtime.compress.colgroup.scheme.ICLAScheme;
+import org.apache.sysds.runtime.compress.cost.ComputationCostEstimator;
+import org.apache.sysds.runtime.data.DenseBlock;
+import org.apache.sysds.runtime.data.SparseBlock;
+import org.apache.sysds.runtime.functionobjects.Builtin;
+import org.apache.sysds.runtime.instructions.cp.CM_COV_Object;
+import org.apache.sysds.runtime.matrix.data.MatrixBlock;
+import org.apache.sysds.runtime.matrix.operators.BinaryOperator;
+import org.apache.sysds.runtime.matrix.operators.CMOperator;
+import org.apache.sysds.runtime.matrix.operators.ScalarOperator;
+import org.apache.sysds.runtime.matrix.operators.UnaryOperator;
+
/**
* Class to encapsulate information about a column group that is first delta
encoded then encoded with dense dictionary
* encoding (DeltaDDC).
*/
-public class ColGroupDeltaDDC { // extends ColGroupDDC
+public class ColGroupDeltaDDC extends AColGroupCompressed implements
AMapToDataGroup {
+
+ private static final long serialVersionUID = -1045556313148564147L;
+ private AMapToData _data;
+ private DeltaDictionary _dict;
+ //TODO Confirm if casting from ADictionary to DeltaDictionary is reasonable
+ private ColGroupDeltaDDC(IColIndex colIndexes, ADictionary dict,
AMapToData data) {
Review Comment:
Change the constructor to take DeltaDictionary, this is a private
constructor so it should be fine.
if you want to you can make another abstract class called ADeltaDictionary
in the dictionary folder that then allows us to extend it. But at the current
time it is optional.
This change allows you to not cast to DeltaDictionary, removing a potential
source of error.
##########
src/main/java/org/apache/sysds/runtime/compress/readers/ReaderColumnSelection.java:
##########
@@ -106,6 +106,30 @@ else if(rawBlock.getDenseBlock().numBlocks() > 1)
return new
ReaderColumnSelectionDenseMultiBlock(rawBlock, colIndices, rl, ru);
return new ReaderColumnSelectionDenseSingleBlock(rawBlock,
colIndices, rl, ru);
}
+ //TODO Add ReaderColumnSelectionDenseDeltaSingleBlock to the method
Review Comment:
add a clean newline after all methods, after this you can add your TODO.
##########
src/main/java/org/apache/sysds/runtime/compress/colgroup/ColGroupDeltaDDC.java:
##########
@@ -19,11 +19,271 @@
package org.apache.sysds.runtime.compress.colgroup;
+
+import org.apache.commons.lang.NotImplementedException;
+import org.apache.sysds.runtime.compress.colgroup.dictionary.ADictionary;
+import org.apache.sysds.runtime.compress.colgroup.dictionary.DeltaDictionary;
+import org.apache.sysds.runtime.compress.colgroup.indexes.IColIndex;
+import org.apache.sysds.runtime.compress.colgroup.mapping.AMapToData;
+import org.apache.sysds.runtime.compress.colgroup.scheme.ICLAScheme;
+import org.apache.sysds.runtime.compress.cost.ComputationCostEstimator;
+import org.apache.sysds.runtime.data.DenseBlock;
+import org.apache.sysds.runtime.data.SparseBlock;
+import org.apache.sysds.runtime.functionobjects.Builtin;
+import org.apache.sysds.runtime.instructions.cp.CM_COV_Object;
+import org.apache.sysds.runtime.matrix.data.MatrixBlock;
+import org.apache.sysds.runtime.matrix.operators.BinaryOperator;
+import org.apache.sysds.runtime.matrix.operators.CMOperator;
+import org.apache.sysds.runtime.matrix.operators.ScalarOperator;
+import org.apache.sysds.runtime.matrix.operators.UnaryOperator;
+
/**
* Class to encapsulate information about a column group that is first delta
encoded then encoded with dense dictionary
* encoding (DeltaDDC).
*/
-public class ColGroupDeltaDDC { // extends ColGroupDDC
+public class ColGroupDeltaDDC extends AColGroupCompressed implements
AMapToDataGroup {
+
+ private static final long serialVersionUID = -1045556313148564147L;
+ private AMapToData _data;
+ private DeltaDictionary _dict;
+ //TODO Confirm if casting from ADictionary to DeltaDictionary is reasonable
+ private ColGroupDeltaDDC(IColIndex colIndexes, ADictionary dict,
AMapToData data) {
+ super(colIndexes);
+ _data = data;
+ _dict = (DeltaDictionary) dict;
+ }
+
+ //TODO Confirm if other parameters for constructors are required, such as
cachedCounts
+ public static AColGroup create(IColIndex colIndexes, ADictionary dict,
AMapToData data) {
+ if(data.getUnique() == 1)
+ return ColGroupConst.create(colIndexes, dict);
+ else if(dict == null)
+ return new ColGroupEmpty(colIndexes);
Review Comment:
not valid.
on the other hand if you observe that all instances point to a dictionary
entry with only 0, then it is valid.
consider the case that all deltas throughout the matrix is 0, then you know
that it is equivalent to an empty matrix.
##########
src/main/java/org/apache/sysds/runtime/compress/colgroup/dictionary/DeltaDictionary.java:
##########
@@ -20,26 +20,90 @@
package org.apache.sysds.runtime.compress.colgroup.dictionary;
import org.apache.commons.lang.NotImplementedException;
-import org.apache.sysds.runtime.functionobjects.Divide;
-import org.apache.sysds.runtime.functionobjects.Minus;
-import org.apache.sysds.runtime.functionobjects.Multiply;
-import org.apache.sysds.runtime.functionobjects.Plus;
+import org.apache.sysds.runtime.compress.colgroup.indexes.IColIndex;
+import org.apache.sysds.runtime.data.SparseBlock;
+import org.apache.sysds.runtime.functionobjects.*;
+import org.apache.sysds.runtime.instructions.cp.CM_COV_Object;
+import org.apache.sysds.runtime.matrix.data.MatrixBlock;
+import org.apache.sysds.runtime.matrix.operators.BinaryOperator;
import org.apache.sysds.runtime.matrix.operators.ScalarOperator;
+import org.apache.sysds.runtime.matrix.operators.UnaryOperator;
+
+import java.io.DataOutput;
+import java.io.IOException;
/**
* This dictionary class is a specialization for the DeltaDDCColgroup. Here
the adjustments for operations for the delta
* encoded values are implemented.
*/
-public class DeltaDictionary extends Dictionary {
+public class DeltaDictionary extends ADictionary {
+
private static final long serialVersionUID = -5700139221491143705L;
private final int _numCols;
+ protected final double[] _values;
+
public DeltaDictionary(double[] values, int numCols) {
- super(values);
+ _values = values;
_numCols = numCols;
}
+ @Override
+ public double[] getValues() {
+ throw new NotImplementedException();
+ }
+
+ @Override
+ public double getValue(int i) {
+ throw new NotImplementedException();
+ }
+
+ @Override
+ public double getValue(int r, int col, int nCol) {
+ throw new NotImplementedException();
+ }
+
+ @Override
+ public long getInMemorySize() {
+ throw new NotImplementedException();
+ }
+
+ @Override
+ public double aggregate(double init, Builtin fn) {
+ throw new NotImplementedException();
+ }
+
+ @Override
+ public double aggregateWithReference(double init, Builtin fn, double[]
reference, boolean def) {
+ throw new NotImplementedException();
+ }
+
+ @Override
+ public double[] aggregateRows(Builtin fn, int nCol) {
+ throw new NotImplementedException();
+ }
+
+ @Override
+ public double[] aggregateRowsWithDefault(Builtin fn, double[]
defaultTuple) {
+ throw new NotImplementedException();
+ }
+
+ @Override
+ public double[] aggregateRowsWithReference(Builtin fn, double[]
reference) {
+ throw new NotImplementedException();
+ }
+
+ @Override
+ public void aggregateCols(double[] c, Builtin fn, IColIndex colIndexes)
{
+ throw new NotImplementedException();
+ }
+
+ @Override
+ public void aggregateColsWithReference(double[] c, Builtin fn,
IColIndex colIndexes, double[] reference, boolean def) {
+ throw new NotImplementedException();
+ }
+
@Override
public DeltaDictionary applyScalarOp(ScalarOperator op) {
Review Comment:
This content of applyScalarOp is not completely valid, if it is not correct,
comment it out and put a NotImplementedException.
##########
src/main/java/org/apache/sysds/runtime/compress/readers/ReaderColumnSelection.java:
##########
@@ -106,6 +106,30 @@ else if(rawBlock.getDenseBlock().numBlocks() > 1)
return new
ReaderColumnSelectionDenseMultiBlock(rawBlock, colIndices, rl, ru);
return new ReaderColumnSelectionDenseSingleBlock(rawBlock,
colIndices, rl, ru);
}
+ //TODO Add ReaderColumnSelectionDenseDeltaSingleBlock to the method
+ public static ReaderColumnSelection createDeltaReader(MatrixBlock
rawBlock, IColIndex colIndices, boolean transposed,
+
int rl, int ru) {
+ checkInput(rawBlock, colIndices, rl, ru);
Review Comment:
The content of this method is not valid, therefore throw
NotImplementedException.
##########
src/main/java/org/apache/sysds/runtime/compress/readers/ReaderColumnSelectionDenseDeltaSingleBlock.java:
##########
@@ -0,0 +1,18 @@
+package org.apache.sysds.runtime.compress.readers;
Review Comment:
Missing license, you can take it from another file.
--
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-unsubscr...@systemds.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
