lindong28 commented on code in PR #253: URL: https://github.com/apache/flink-ml/pull/253#discussion_r1317198364
########## flink-ml-lib/src/main/java/org/apache/flink/ml/anomalydetection/isolationforest/ITree.java: ########## @@ -0,0 +1,156 @@ +/* + * 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.flink.ml.anomalydetection.isolationforest; + +import org.apache.flink.api.java.tuple.Tuple2; +import org.apache.flink.ml.linalg.DenseVector; + +import java.io.Serializable; +import java.util.Random; + +/** Construct isolation tree. */ +public class ITree implements Serializable { + public final int attributeIndex; + public final double splitAttributeValue; + public ITree leftTree; + public ITree rightTree; + public int currentHeight; + public int leafNodesNum; + + public ITree(int attributeIndex, double splitAttributeValue) { + this.attributeIndex = attributeIndex; + this.splitAttributeValue = splitAttributeValue; + this.leftTree = null; + this.rightTree = null; + this.currentHeight = 0; + this.leafNodesNum = 1; + } + + public static ITree generateIsolationTree( + DenseVector[] samplesData, + int currentHeight, + int limitHeight, + Random randomState, + int[] featureIndices) { + int n = samplesData.length; + ITree isolationTree; + if (samplesData.length == 0) { + return null; + } else if (samplesData.length == 1 || currentHeight >= limitHeight) { + isolationTree = new ITree(0, samplesData[0].get(0)); + isolationTree.currentHeight = currentHeight; + isolationTree.leafNodesNum = samplesData.length; + return isolationTree; + } + boolean flag = true; + for (int i = 1; i < n; i++) { + if (!samplesData[i].equals(samplesData[i - 1])) { + flag = false; + break; + } + } + if (flag) { + isolationTree = new ITree(0, samplesData[0].get(0)); + isolationTree.currentHeight = currentHeight; + isolationTree.leafNodesNum = samplesData.length; + return isolationTree; + } + + Tuple2<Integer, Double> tuple2 = + getRandomFeatureToSplit(samplesData, randomState, featureIndices); + int attributeIndex = tuple2.f0; + double splitAttributeValue = tuple2.f1; + + int leftNodesNum = 0; + int rightNodesNum = 0; + for (DenseVector datum : samplesData) { + if (datum.get(attributeIndex) < splitAttributeValue) { + leftNodesNum++; + } else { + rightNodesNum++; + } + } + + DenseVector[] leftSamples = new DenseVector[leftNodesNum]; + DenseVector[] rightSamples = new DenseVector[rightNodesNum]; + int l = 0, r = 0; + for (DenseVector samplesDatum : samplesData) { + if (samplesDatum.get(attributeIndex) < splitAttributeValue) { + leftSamples[l++] = samplesDatum; + } else { + rightSamples[r++] = samplesDatum; + } + } + + ITree root = new ITree(attributeIndex, splitAttributeValue); + root.currentHeight = currentHeight; + root.leafNodesNum = samplesData.length; + root.leftTree = + generateIsolationTree( + leftSamples, currentHeight + 1, limitHeight, randomState, featureIndices); + root.rightTree = + generateIsolationTree( + rightSamples, currentHeight + 1, limitHeight, randomState, featureIndices); + + return root; + } + + private static Tuple2<Integer, Double> getRandomFeatureToSplit( + DenseVector[] samplesData, Random randomState, int[] featureIndices) { + int attributeIndex = featureIndices[randomState.nextInt(featureIndices.length)]; + + double maxValue = samplesData[0].get(attributeIndex); + double minValue = samplesData[0].get(attributeIndex); + for (int i = 1; i < samplesData.length; i++) { + minValue = Math.min(minValue, samplesData[i].get(attributeIndex)); + maxValue = Math.max(maxValue, samplesData[i].get(attributeIndex)); + } + double splitAttributeValue = (maxValue - minValue) * randomState.nextDouble() + minValue; + + return Tuple2.of(attributeIndex, splitAttributeValue); + } + + public static double calculatePathLength(DenseVector sampleData, ITree isolationTree) + throws Exception { + double pathLength = -1; + ITree tmpITree = isolationTree; + while (tmpITree != null) { + pathLength += 1; + if (tmpITree.leftTree == null + || tmpITree.rightTree == null + || sampleData.get(tmpITree.attributeIndex) == tmpITree.splitAttributeValue) { + break; + } else if (sampleData.get(tmpITree.attributeIndex) < tmpITree.splitAttributeValue) { + tmpITree = tmpITree.leftTree; + } else { + tmpITree = tmpITree.rightTree; + } + } + + assert tmpITree != null; Review Comment: In general we only use `assert` in unit tests. Given that we will have NullPointerException in the line below if `tmpITree == null`, it seems simpler to just remove this line. ########## flink-ml-lib/src/main/java/org/apache/flink/ml/anomalydetection/isolationforest/IsolationForestParams.java: ########## @@ -0,0 +1,66 @@ +/* + * 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.flink.ml.anomalydetection.isolationforest; + +import org.apache.flink.ml.param.DoubleParam; +import org.apache.flink.ml.param.IntParam; +import org.apache.flink.ml.param.Param; + +/** + * Params of {@link IsolationForest}. + * + * @param <T> The class of this instance. + */ +public interface IsolationForestParams<T> extends IsolationForestModelParams<T> { + Param<Integer> MAX_SAMPLES = + new IntParam( + "maxSamples", + "The number of samplesData to train and its max value is preferably 256.", + 256); + + Param<Double> MAX_FEATURES = + new DoubleParam( + "maxFeatures", + "The number of features used to train each tree and it is treated as a fraction in the range (0, 1.0].", + 1.0); + + default int getNumTrees() { Review Comment: Given that this method is already defined in `IsolationForestModelParams`, should we remove it here? Same for `setNumTrees()`. ########## flink-ml-lib/src/main/java/org/apache/flink/ml/anomalydetection/isolationforest/ITree.java: ########## @@ -0,0 +1,156 @@ +/* + * 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.flink.ml.anomalydetection.isolationforest; + +import org.apache.flink.api.java.tuple.Tuple2; +import org.apache.flink.ml.linalg.DenseVector; + +import java.io.Serializable; +import java.util.Random; + +/** Construct isolation tree. */ +public class ITree implements Serializable { + public final int attributeIndex; + public final double splitAttributeValue; + public ITree leftTree; + public ITree rightTree; + public int currentHeight; Review Comment: Given that we only mutate `currentHeight` right after it is constructed, it should be possible and better to make it `final`. Same for the other three non-final variables. ########## flink-ml-lib/src/main/java/org/apache/flink/ml/anomalydetection/isolationforest/IForest.java: ########## @@ -0,0 +1,137 @@ +/* + * 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.flink.ml.anomalydetection.isolationforest; + +import org.apache.flink.ml.linalg.DenseVector; + +import java.io.Serializable; +import java.util.ArrayList; +import java.util.List; +import java.util.Random; + +/** Construct isolation forest. */ +public class IForest implements Serializable { + public final int numTrees; + public List<ITree> iTreeList; + public Double center0; + public Double center1; + public int subSamplesSize; + + public IForest(int numTrees) { + this.numTrees = numTrees; + this.iTreeList = new ArrayList<>(256); + this.center0 = null; + this.center1 = null; + } + + public void generateIsolationForest(DenseVector[] samplesData, int[] featureIndices) { + int n = samplesData.length; + this.subSamplesSize = Math.min(256, n); Review Comment: nits: would it be more consistent with existing code to use `subSamplesSize` directly here? Same for other usages of `this.*` outside constructor. -- 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]
