On Sun, Mar 12, 2017 at 11:19 PM, Peter Thygesen <[email protected] > wrote:
> Hi OpenNLP Developers > > > > After I some weeks ago added a PR for NameFinderSequenceValidator test, I > spent some time looking into the BilouNameFinderSequenceValidator and the > Codec classes, trying to understand how they work and I wrote some more > tests that were missing (OPENNLP-1000) > > > > I have a few comments and questions I hope someone here can answer. > > > > General question: > > Why are NameFinder.START and NameFinder.CONTINUE used I the codec classes > and namefindervalidator classes when BioCodec/BilouCodec have their own > tags? > > NameFinder should not have them after my opinion. They belong to the Codec > classes. > Yes, that is how it should be. The used to be part of the NameFinder but then were moved to their own class. It would be good to refactor that. > > The BIO doesn’t really match the “Start”, “Continue”, “Other” tags.. even > though its “similar”. And for the BILOU its mixed opennlp+bilou: > Start,Continue,Last,Unit,Other… Some what messy I think. But not BIG thing > for me, and yes BILOU also have many names.. (like BMEWO) > The tags were never renamed because that would break backward compatibility with existing models. > > Class: BioCodec > > Method: areOutcomesCompatible(String[] outcomes) > > > > I assume that this method is checking that tags in the model are compatible > with opennlp tags (from the codec). And that it receives an array for > unique tags in the model. > > Is this true? > > Yes > > The comment in the beginning of the method says: ”… To validate the model > we check if we have one outcome named ”other”… > > However, this is not directly checked. The current test will pass outcomes > without “other” if the outcomes have other valid tags. > > So, can a model be compatible without an “other” tag? > > > E.g. > > Are outcomes=[“person-start”], [“person-start”, “person-continue”] or > [“person-start”, “location-start”] compatible models? > > This could be the case, in practice this all probably doesn't matter that much. This check should fail if a user somehow ends up with a model which doesn't come from the name finder, e.g. a pos tagger maxent model shouldn't be validated as valid by this method. > > The method could use Set instead of Lists (yes.. the lists are probably > small for single class models) > > > > Class: BilouCodec > > Method: areOutcomesCompatiple(String[] outcomes) implementation is missing. > > I made one which you can have and review. > > > This would be great if you could contribute it. > > Like the BioCodec I need to know whether a model is compatible without the > “other” tag. > > I think having a valid model with out other is ok. > > Class: BilouNameFinderValidator > > I have written a unit test (OPENNLP-1000) for this class and it shows its > buggy. I will soon push a PR for this. > > Failed tests: (should not be valid) > > “Start” followed by “Start” > > “Start” followed by “Other” > > “Start” followed by “Unit” > > “Continue” followed by “Start” > > “Continue” followed by “Unit” > > “Last” followed by “Continue” > > “Last” followed by “Last” > > I reviewed your PR and this looks good. I need to run the evaluation tests once with that PR and then we can merge it. > > I can create some JIRA and PR for my test if you like. (I hope you like :-) > > Please do that and send us more PRs, all you contributions are very welcome and especially writing tests helps us a lot to improve code quality. Jörn
