Thx for the reply. BTW BioCodec also contains a static method: String extractNameType(...) this method should be move out. Perhaps to a base class called Codec (or CodecBase... or how you normally named base classes) The method is also called by BilouCodec, which then is calling directly to BioCodec... (which smells)
Perhaps another refactoring task? /Peter 2017-03-14 12:48 GMT+01:00 Joern Kottmann <[email protected]>: > 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 >
