-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-24817] Add Estimator and Transformer for Naive Bayes #32
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, Yunfeng!
Overall it looks good to me. I left some minor comments here.
By the way, could you also reformat the code by mvn spotless:apply
?
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/common/datastream/EndOfStreamWindows.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Outdated
Show resolved
Hide resolved
...-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModelData.java
Outdated
Show resolved
Hide resolved
...-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModelData.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. Left some comments below.
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesParams.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesParams.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesParams.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Outdated
Show resolved
Hide resolved
f45c219
to
17a8659
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. Left some comments below.
It looks like mvn package
fails due to stylecheck error. Could you make sure mvn package
works?
flink-ml-lib/src/main/java/org/apache/flink/ml/common/param/HasLabelCol.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/common/param/HasFeatureSize.java
Outdated
Show resolved
Hide resolved
...l-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModelParams.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/test/java/org/apache/flink/ml/classification/NaiveBayesTest.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/test/java/org/apache/flink/ml/classification/NaiveBayesTest.java
Show resolved
Hide resolved
flink-ml-lib/src/test/java/org/apache/flink/ml/classification/NaiveBayesTest.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/test/java/org/apache/flink/ml/classification/NaiveBayesTest.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModel.java
Outdated
Show resolved
Hide resolved
17a8659
to
f9898b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. Left some comments below.
flink-ml-lib/src/test/java/org/apache/flink/ml/classification/NaiveBayesTest.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/test/java/org/apache/flink/ml/classification/NaiveBayesTest.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/test/java/org/apache/flink/ml/classification/NaiveBayesTest.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/test/java/org/apache/flink/ml/classification/NaiveBayesTest.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModel.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModel.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModel.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModel.java
Outdated
Show resolved
Hide resolved
f9898b9
to
87fecca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update @yunfengzhou-hub. I left some minor comments regarding the code readability.
flink-ml-lib/src/test/java/org/apache/flink/ml/clustering/KMeansTest.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/test/java/org/apache/flink/ml/clustering/KMeansTest.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/test/java/org/apache/flink/ml/clustering/KMeansTest.java
Show resolved
Hide resolved
flink-ml-lib/src/test/java/org/apache/flink/ml/clustering/KMeansTest.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/test/java/org/apache/flink/ml/util/StageTestUtils.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModel.java
Outdated
Show resolved
Hide resolved
27b59b0
to
fdd2729
Compare
flink-ml-api/src/main/java/org/apache/flink/ml/linalg/BLAS.java
Outdated
Show resolved
Hide resolved
f378fc2
to
a2fa962
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update @yunfengzhou-hub. Left only some minor comments below.
flink-ml-api/src/main/java/org/apache/flink/ml/linalg/BLAS.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/test/java/org/apache/flink/ml/util/StageTestUtils.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/test/java/org/apache/flink/ml/util/StageTestUtils.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/test/java/org/apache/flink/ml/classification/NaiveBayesTest.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/test/java/org/apache/flink/ml/classification/NaiveBayesTest.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModel.java
Outdated
Show resolved
Hide resolved
a2fa962
to
aabb2e3
Compare
Thanks for the update! LGTM. @gaoyunhaii could you help review this PR? |
a550979
to
de2563b
Compare
...-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModelData.java
Outdated
Show resolved
Hide resolved
...-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModelData.java
Show resolved
Hide resolved
de2563b
to
de88a30
Compare
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModel.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModel.java
Outdated
Show resolved
Hide resolved
...-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModelData.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModel.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Outdated
Show resolved
Hide resolved
flink-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayes.java
Outdated
Show resolved
Hide resolved
flink-ml-core/src/main/java/org/apache/flink/ml/linalg/BLAS.java
Outdated
Show resolved
Hide resolved
...-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModelData.java
Show resolved
Hide resolved
...-ml-lib/src/main/java/org/apache/flink/ml/classification/naivebayes/NaiveBayesModelData.java
Outdated
Show resolved
Hide resolved
ff097e8
to
901bcd8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very thanks @yunfengzhou-hub for the updates! LGTM and +1 to merge
901bcd8
to
40f1bf1
Compare
What is the purpose of the change
This PR adds the implementation of Naive Bayes algorithm to Flink ML. This algorithm is implemented in reference to that implemented in Alibaba Alink and Apache Spark, but it uses Flink ML's framework and Flink Datastream.
Brief change log
This PR adds public classes NaiveBayes. Users can use NaiveBayes to do training and inference according to the algorithm with the same name. Users can also use MultiStringIndexer to convert strings into indices.
Verifying this change
The changes are tested by unit tests in NaiveBayesTest.
Does this pull request potentially affect one of the following parts:
Dependencies (does it add or upgrade a dependency): (yes)
The public API, i.e., is any changed class annotated with @public(Evolving): (yes)
Documentation
Does this pull request introduce a new feature? (yes)
If yes, how is the feature documented? (Java doc)