[FLINK-33003] Support isolation forest algorithm in Flink ML #253

zhaozijun109 · 2023-08-31T10:01:15Z

What is the purpose of the change

This pull request adds the implementation of Isolation forest algorithm.

Brief change log

Adds Isolation forest algorithm inJava (flink-ml-lib)

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (no)

Documentation

Does this pull request introduce a new feature? (no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

zhaozijun109 · 2023-09-01T02:41:05Z

Hello @lindong28 @zhipeng93 teachers, this problem seems that the beam package in pyflink has changed. How should I solve this problem?

lindong28

Thanks for the PR! Left some comments below.

Can you help make sure that all tests can pass successfully?

...-lib/src/main/java/org/apache/flink/ml/anomalydetection/isolationforest/IsolationForest.java

...rc/main/java/org/apache/flink/ml/anomalydetection/isolationforest/IsolationForestParams.java

...src/main/java/org/apache/flink/ml/anomalydetection/isolationforest/IsolationForestModel.java

...-lib/src/main/java/org/apache/flink/ml/anomalydetection/isolationforest/IsolationForest.java

lindong28 · 2023-09-02T15:11:13Z

...-lib/src/main/java/org/apache/flink/ml/anomalydetection/isolationforest/IsolationForest.java

+    public static class ITree implements Serializable {
+        public int attributeIndex;
+        public double splitAttributeValue;
+        public ITree leftTree, rightTree;


Can we declare one variable at a line for consistency with the existing code style?

...main/java/org/apache/flink/ml/anomalydetection/isolationforest/IsolationForestModelData.java

...-lib/src/main/java/org/apache/flink/ml/anomalydetection/isolationforest/IsolationForest.java

…ut the IForest and ITree in IsolationForest and add test case.)

zhaozijun109 · 2023-09-06T03:02:30Z

@lindong28 Please review again teacher，thank you.

lindong28

Thanks for your contribution and the PR update! I left some comments below.

Sorry, I might not have time to actively review this PR later. I will ask if some of my colleagues can help review this PR.

The PR currently fails compilation due to the following error in the log [1]. I don't think it is related to either pyflink or beam. But I probably won't be able to dig into this anytime soon.

Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:compile (default-compile) on project flink-ml-lib-1.17: Compilation failure: Compilation failure: 
Error:  /home/runner/work/flink-ml/flink-ml/flink-ml-lib/src/main/java/org/apache/flink/ml/anomalydetection/isolationforest/IsolationForest.java:[302,21] cannot infer type arguments for org.apache.flink.iteration.datacache.nonkeyed.ListStateWithCache<>
Error:    reason: cannot infer type-variable(s) T
Error:      (actual and formal argument lists differ in length)

[1] https://github.com/apache/flink-ml/actions/runs/6092176700/job/16543229469?pr=253

lindong28 · 2023-09-06T12:10:27Z

...rc/main/java/org/apache/flink/ml/anomalydetection/isolationforest/IsolationForestParams.java

+                    "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() {


Given that this method is already defined in IsolationForestModelParams, should we remove it here?

Same for setNumTrees().

Yes, teacher, I have deleted him in second pr which will overwrite the first pr.

lindong28 · 2023-09-06T12:17:39Z

flink-ml-lib/src/main/java/org/apache/flink/ml/anomalydetection/isolationforest/IForest.java

+
+    public void generateIsolationForest(DenseVector[] samplesData, int[] featureIndices) {
+        int n = samplesData.length;
+        this.subSamplesSize = Math.min(256, n);


nits: would it be more consistent with existing code to use subSamplesSize directly here?

Same for other usages of this.* outside constructor.

OK, I will fix it.

lindong28 · 2023-09-06T12:22:55Z

flink-ml-lib/src/main/java/org/apache/flink/ml/anomalydetection/isolationforest/ITree.java

+            }
+        }
+
+        assert tmpITree != null;


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.

Ok, I will delete it.

lindong28 · 2023-09-06T12:26:03Z

flink-ml-lib/src/main/java/org/apache/flink/ml/anomalydetection/isolationforest/ITree.java

+    public final double splitAttributeValue;
+    public ITree leftTree;
+    public ITree rightTree;
+    public int currentHeight;


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.

Sounds good for me, I will make it final.

zhaozijun109 · 2023-09-07T02:23:08Z

Thanks for your contribution and the PR update! I left some comments below.

Sorry, I might not have time to actively review this PR later. I will ask if some of my colleagues can help review this PR.

The PR currently fails compilation due to the following error in the log [1]. I don't think it is related to either pyflink or beam. But I probably won't be able to dig into this anytime soon.
Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:compile (default-compile) on project flink-ml-lib-1.17: Compilation failure: Compilation failure: 
Error:  /home/runner/work/flink-ml/flink-ml/flink-ml-lib/src/main/java/org/apache/flink/ml/anomalydetection/isolationforest/IsolationForest.java:[302,21] cannot infer type arguments for org.apache.flink.iteration.datacache.nonkeyed.ListStateWithCache<>
Error:    reason: cannot infer type-variable(s) T
Error:      (actual and formal argument lists differ in length)
[1] https://github.com/apache/flink-ml/actions/runs/6092176700/job/16543229469?pr=253

Teacher, this problem seems to be ListStateWithCache class. I am not sure if anyone has submitted the relevant PR. I will rebuild the branch based on the latest master, test it and resubmit it.Thank you for your suggestion and review, best to you.

zhaozijun109 · 2023-09-07T02:33:20Z

Thanks for your contribution and the PR update! I left some comments below.

Sorry, I might not have time to actively review this PR later. I will ask if some of my colleagues can help review this PR.

The PR currently fails compilation due to the following error in the log [1]. I don't think it is related to either pyflink or beam. But I probably won't be able to dig into this anytime soon.
Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:compile (default-compile) on project flink-ml-lib-1.17: Compilation failure: Compilation failure: 
Error:  /home/runner/work/flink-ml/flink-ml/flink-ml-lib/src/main/java/org/apache/flink/ml/anomalydetection/isolationforest/IsolationForest.java:[302,21] cannot infer type arguments for org.apache.flink.iteration.datacache.nonkeyed.ListStateWithCache<>
Error:    reason: cannot infer type-variable(s) T
Error:      (actual and formal argument lists differ in length)
[1] https://github.com/apache/flink-ml/actions/runs/6092176700/job/16543229469?pr=253

This is due to this PR (ebdf362) induced，I will fix the problem.

[FLINK-33003] Support isolation forest algorithm in Flink ML

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

a2c2eba

lindong28 reviewed Sep 2, 2023

View reviewed changes

[FLINK-33003] Support isolation forest algorithm in Flink ML.(Split o…

8978bb4

…ut the IForest and ITree in IsolationForest and add test case.)

lindong28 reviewed Sep 6, 2023

View reviewed changes

zhaozijun109 requested a review from lindong28 September 7, 2023 07:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-33003] Support isolation forest algorithm in Flink ML #253

[FLINK-33003] Support isolation forest algorithm in Flink ML #253

zhaozijun109 commented Aug 31, 2023 •

edited

Loading

zhaozijun109 commented Sep 1, 2023 •

edited

Loading

lindong28 left a comment •

edited

Loading

lindong28 Sep 2, 2023

zhaozijun109 Sep 4, 2023

zhaozijun109 commented Sep 6, 2023

lindong28 left a comment

lindong28 Sep 6, 2023

zhaozijun109 Sep 7, 2023

lindong28 Sep 6, 2023

zhaozijun109 Sep 7, 2023

lindong28 Sep 6, 2023

zhaozijun109 Sep 7, 2023

lindong28 Sep 6, 2023

zhaozijun109 Sep 7, 2023

zhaozijun109 commented Sep 7, 2023 •

edited

Loading

zhaozijun109 commented Sep 7, 2023

[FLINK-33003] Support isolation forest algorithm in Flink ML #253

Are you sure you want to change the base?

[FLINK-33003] Support isolation forest algorithm in Flink ML #253

Conversation

zhaozijun109 commented Aug 31, 2023 • edited Loading

What is the purpose of the change

Brief change log

Does this pull request potentially affect one of the following parts:

Documentation

zhaozijun109 commented Sep 1, 2023 • edited Loading

lindong28 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaozijun109 commented Sep 6, 2023

lindong28 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaozijun109 commented Sep 7, 2023 • edited Loading

zhaozijun109 commented Sep 7, 2023

zhaozijun109 commented Aug 31, 2023 •

edited

Loading

zhaozijun109 commented Sep 1, 2023 •

edited

Loading

lindong28 left a comment •

edited

Loading

zhaozijun109 commented Sep 7, 2023 •

edited

Loading