lindong28 commented on code in PR #103:
URL: https://github.com/apache/flink-ml/pull/103#discussion_r880067284
##########
flink-ml-lib/src/test/java/org/apache/flink/ml/classification/KnnTest.java:
##########
@@ -121,8 +124,9 @@ private static void verifyPredictionResult(Table output,
String labelCol, String
@Override
public Tuple2<Double, Double> map(Row row)
{
return Tuple2.of(
- (Double)
row.getField(labelCol),
- (Double)
row.getField(predictionCol));
+ ((Number)
row.getField(labelCol)).doubleValue(),
Review Comment:
The test code basically should mimic the user code's beahvior.
Since we have updated the stage to output double-typed label column, do we
still need to do type conversion here using `Number::doubleValue()`?
Same for prediction column.
Same for `LinearSVCTest`.
##########
flink-ml-lib/src/test/java/org/apache/flink/ml/classification/KnnTest.java:
##########
@@ -179,6 +183,42 @@ public void testFitAndPredict() throws Exception {
verifyPredictionResult(output, knn.getLabelCol(),
knn.getPredictionCol());
}
+ @Test
+ public void testDataTypes() throws Exception {
Review Comment:
nits: would it be more intuitive to rename this test as
`testInputTypeConversion()`?
Same for `LinearSVCTest`.
##########
flink-ml-core/src/main/java/org/apache/flink/ml/linalg/DenseVector.java:
##########
@@ -51,6 +51,35 @@ public double[] toArray() {
return values;
}
+ @Override
+ public DenseVector toDense() {
+ return this;
+ }
+
+ @Override
+ public SparseVector toSparse() {
+ int nnz = 0;
Review Comment:
I am not sure `nnz` is an intuitive name. The general rule is to make
variable names either self-explanatory or simple.
Could we change this to either `non_zero_count` or `count`?
Same for `ii` and `vv`.
--
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]
