jiangxin369 commented on code in PR #219:
URL: https://github.com/apache/flink-ml/pull/219#discussion_r1139895407
##########
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/logisticregression/LogisticRegressionModel.java:
##########
@@ -149,22 +152,14 @@ public Row map(Row dataPoint) {
getRuntimeContext().getBroadcastVariable(broadcastModelKey).get(0);
coefficient = modelData.coefficient;
}
- DenseVector features = ((Vector)
dataPoint.getField(featuresCol)).toDense();
- Row predictionResult = predictOneDataPoint(features, coefficient);
- return Row.join(dataPoint, predictionResult);
- }
- }
+ Vector features = (Vector) dataPoint.getField(featuresCol);
+
+ LogisticRegressionModelServable servable =
+ new LogisticRegressionModelServable(
Review Comment:
1. I think we'd better keep the constructor but make it protected or
default. Because without the
`LogisticRegressionModelServable(LogisticRegressionModelData)` constructor,
every time we want to construct the `LogisticRegressionModelServable` with an
existing `LogisticRegressionModelData`, we need to serialize the instance of
`LogisticRegressionModelData` to bytes and then deserialize the bytes via
`setModelData(InputStream... modelDataInputs)`, it may be expensive if the
model data is large.
2. Yes we should forward parameters from model to servable, but the
`LogisticRegressionModelServable` doesn't need any parameters from model to do
predictions. If we are supporting `MinMaxScalerModelServable`, developers can
pass the params by `servable.setMin()` directly.
--
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]
