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

OPENNLP-229: Write a test case for the NameFinderSequenceValidator class #125

Closed
wants to merge 1 commit into from

Conversation

thygesen
Copy link
Contributor

I have written the missing test for NameFinderSequenceValidator. Please note that the test:testContinueAfterStartAndNotSameType has temporary been disabled because it will fail due to a minor "bug" in the NameFinderSequenceValidator. At the moment the validator accepts Continue after Start even though they are different name types.

Copy link
Member

@wcolen wcolen left a comment

Choose a reason for hiding this comment

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

Great! I was missing it in your other PR.

private static NameFinderSequenceValidator validator = new NameFinderSequenceValidator();
private static String START_A = "TypeA-" + NameFinderME.START;
private static String CONTINUE_A = "TypeA-" + NameFinderME.CONTINUE;
private static String OTHER_A = "TypeA-" + NameFinderME.OTHER;
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if it make sense to concat "TypeA-" + NameFinderME.OTHER, we usually have only NameFinderME.OTHER

Copy link
Contributor Author

@thygesen thygesen Feb 15, 2017

Choose a reason for hiding this comment

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

I need more that one name type for the test and therefore I can not just use NameFinderME.START/OTHER/CONT. Running on some test data showed me that the Name Type is added in front of the "outcome" (or "default-" is added.)

Copy link
Member

@kottmann kottmann Feb 16, 2017

Choose a reason for hiding this comment

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

What he means is that there is no OTHER_A or OTHER_B it is always just OTHER. Indicating that none of the types in the model cover that particular token.


}

@Ignore
Copy link
Member

Choose a reason for hiding this comment

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

That is a good solution here. We can merge this PR and then the other PR with the bug fix can enable the test case.

@kottmann
Copy link
Member

Commit message in imperative and it should contain the jira:
OPENNLP-229: Add test for NameFinderSequenceValidator

And a easy way to update an existing PR is:

  • git commit --amend (to add some changes to the last commit)
  • git push -f (to overwrite your remote branch with the new commit)

@thygesen thygesen force-pushed the opennlp-229 branch 2 times, most recently from 2f5673b to 93bdf5d Compare February 17, 2017 08:08
OPENNLP-229: Add test for NameFinderSequenceValidator

OPENNLP-229: Add test for NameFinderSequenceValidator
Copy link
Member

@kottmann kottmann left a comment

Choose a reason for hiding this comment

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

+1 lgtm

@wcolen
Copy link
Member

wcolen commented Feb 17, 2017

+1 lgtm

@asfgit asfgit closed this in c5a15b2 Feb 17, 2017
@kottmann
Copy link
Member

Thanks for contributing this. I merged this now and edited your commit message a bit (it contained multiple identical lines)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants