You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "Fanoid (via GitHub)" <gi...@apache.org> on 2023/02/07 03:08:31 UTC

[GitHub] [flink-ml] Fanoid opened a new pull request, #205: [FLINK-30730] Fix null values handling in StringIndexer and StringIndexerModel

Fanoid opened a new pull request, #205:
URL: https://github.com/apache/flink-ml/pull/205

   ## What is the purpose of the change
   
   This PR fixes null values handling in StringIndexer and StringIndexerModel.
   
   ## Brief change log
   
    - Skip null values in training, and handle null values in transforming.
    - Add tests for null values handling in corresponding test class.
   
   ## 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
   


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-ml] zhipeng93 commented on a diff in pull request #205: [FLINK-30730] Fix null values handling in StringIndexer and StringIndexerModel

Posted by "zhipeng93 (via GitHub)" <gi...@apache.org>.
zhipeng93 commented on code in PR #205:
URL: https://github.com/apache/flink-ml/pull/205#discussion_r1098259151


##########
flink-ml-lib/src/test/java/org/apache/flink/ml/feature/stringindexer/StringIndexerTest.java:
##########
@@ -53,6 +55,9 @@ public class StringIndexerTest extends AbstractTestBase {
     private Table trainTable;
     private Table predictTable;
 
+    private Table trainTableWithNull;

Review Comment:
   Thanks for the PR. The fix looks good to me. One minor question: Can you simplify the test by adding null values in `trainTable` and `predictTable`? In this case, the test case would be simpler. And it would be great if we also update the python test.



##########
flink-ml-lib/src/test/java/org/apache/flink/ml/feature/stringindexer/StringIndexerTest.java:
##########
@@ -53,6 +55,9 @@ public class StringIndexerTest extends AbstractTestBase {
     private Table trainTable;
     private Table predictTable;
 
+    private Table trainTableWithNull;

Review Comment:
   Thanks for the PR. The fix looks good to me. 
   
   One minor question: Can you simplify the test by adding null values in `trainTable` and `predictTable`? In this case, the test case would be simpler. And it would be great if we also update the python test.



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-ml] zhipeng93 commented on pull request #205: [FLINK-30730] Fix null values handling in StringIndexer and StringIndexerModel

Posted by "zhipeng93 (via GitHub)" <gi...@apache.org>.
zhipeng93 commented on PR #205:
URL: https://github.com/apache/flink-ml/pull/205#issuecomment-1420609867

   Thanks for the update. LGTM.


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-ml] zhipeng93 commented on a diff in pull request #205: [FLINK-30730] Fix null values handling in StringIndexer and StringIndexerModel

Posted by "zhipeng93 (via GitHub)" <gi...@apache.org>.
zhipeng93 commented on code in PR #205:
URL: https://github.com/apache/flink-ml/pull/205#discussion_r1098259151


##########
flink-ml-lib/src/test/java/org/apache/flink/ml/feature/stringindexer/StringIndexerTest.java:
##########
@@ -53,6 +55,9 @@ public class StringIndexerTest extends AbstractTestBase {
     private Table trainTable;
     private Table predictTable;
 
+    private Table trainTableWithNull;

Review Comment:
   Thanks for the PR. The fix looks good to me. 
   
   One minor question: Can you simplify the test by adding null values in `trainTable` and `predictTable`? And it would be great if we also update the python test.



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-ml] zhipeng93 commented on a diff in pull request #205: [FLINK-30730] Fix null values handling in StringIndexer and StringIndexerModel

Posted by "zhipeng93 (via GitHub)" <gi...@apache.org>.
zhipeng93 commented on code in PR #205:
URL: https://github.com/apache/flink-ml/pull/205#discussion_r1098259151


##########
flink-ml-lib/src/test/java/org/apache/flink/ml/feature/stringindexer/StringIndexerTest.java:
##########
@@ -53,6 +55,9 @@ public class StringIndexerTest extends AbstractTestBase {
     private Table trainTable;
     private Table predictTable;
 
+    private Table trainTableWithNull;

Review Comment:
   Can you simplify the test by adding null values in `trainTable` and `predictTable`? In this case, the test case would be simpler.
   
   Moreover, it would be great if we also update the python test.



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-ml] zhipeng93 merged pull request #205: [FLINK-30730] Fix null values handling in StringIndexer and StringIndexerModel

Posted by "zhipeng93 (via GitHub)" <gi...@apache.org>.
zhipeng93 merged PR #205:
URL: https://github.com/apache/flink-ml/pull/205


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-ml] Fanoid commented on a diff in pull request #205: [FLINK-30730] Fix null values handling in StringIndexer and StringIndexerModel

Posted by "Fanoid (via GitHub)" <gi...@apache.org>.
Fanoid commented on code in PR #205:
URL: https://github.com/apache/flink-ml/pull/205#discussion_r1098429500


##########
flink-ml-lib/src/test/java/org/apache/flink/ml/feature/stringindexer/StringIndexerTest.java:
##########
@@ -53,6 +55,9 @@ public class StringIndexerTest extends AbstractTestBase {
     private Table trainTable;
     private Table predictTable;
 
+    private Table trainTableWithNull;

Review Comment:
   Thanks for your suggestion. I've updated the PR.  Please check the changes.



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org