Skip to content
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

Included StringIndexer and StringIndexerModel along with related test… #804

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

ramanathanv
Copy link
Contributor

Hi,

I am creating this pull request that implements the basic methods in StringIndexer and StringIndexerModel class along with Test cases.

This is related to "Implement ML Features" #381

Comment on lines 58 to 59
/// <param name="source"></param>
/// <returns></returns>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add the param and returns description?

@Niharikadutta
Copy link
Collaborator

@ramanathanv Is this PR ready to review? I see some StringIndexerTests tests failing in the pipeline.

@ramanathanv
Copy link
Contributor Author

@ramanathanv Is this PR ready to review? I see some StringIndexerTests tests failing in the pipeline.

Hi @Niharikadutta ,

In the test case, I am trying to compare two List objects using Assert.Equal(). I notice in the test results that the Lists have the same content (see image below). Still there is an error.
image

I assume that there must be a different way to compare the Lists. Can you please suggest me a way an alternate way to do this comparison?

Thanks.

@imback82
Copy link
Contributor

I notice in the test results that the Lists have the same content (see image below).

I see "..." in the output. Can you compare elements one by one to see where the difference comes from?

@ramanathanv
Copy link
Contributor Author

I notice in the test results that the Lists have the same content (see image below).

I see "..." in the output. Can you compare elements one by one to see where the difference comes from?

Hi @imback82 ,
I noticed that even the raw log file provides only 3 dots "...". Is there another way for me to check this?

@imback82
Copy link
Contributor

What I meant was if you are comparing two lists, you can compare elements in those lists one by one instead of comparing them at the list level. Does it make sense?

@GoEddie
Copy link
Contributor

GoEddie commented Feb 1, 2021

In StringIndexer there are a few calls which are missing the get* at the beginning - if I change them all to get* then the test passes for me.

public string GetStringOrderType() => (string)_jvmObject.Invoke("stringOrderType");

should be

public string GetStringOrderType() => (string)_jvmObject.Invoke("getStringOrderType");
public string[] GetInputCols() => (string[])_jvmObject.Invoke("inputCols");

should be

public string[] GetInputCols() => (string[])_jvmObject.Invoke("getInputCols");
public string GetInputCol() => (string)_jvmObject.Invoke("inputCol");

should be

public string GetInputCol() => (string)_jvmObject.Invoke("getInputCol");
public string GetHandleInvalid() => (string)_jvmObject.Invoke("handleInvalid");

should be

public string GetHandleInvalid() => (string)_jvmObject.Invoke("getHandleInvalid");

If you change those then the tests should pass :)

@ramanathanv
Copy link
Contributor Author

If you change those then the tests should pass :)

Thanks @GoEddie for identifying the issue . I have fixed the property names. But the test fails for unknown reasons. Can you please re-initiate the build/tests?

Base automatically changed from master to main March 18, 2021 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants