The references in BioCodec and BilouCodec to NameFinderME tags are easy to replace, but it looks difficult to get rid of the NameFinderME.OTHER tags in DefaultNameContextGenerator.getContext(..) because it is not obvious to me from which Codec the tags would come from. Also NameFinderEventStream.genereateOutcomes(..) has references to NameFinderME tags. Here it looks lige the tags comes from BioCodec. The method is however deprecated and should probably just be as it is untill "removed".
/Peter 2017-03-15 9:53 GMT+01:00 Joern Kottmann <[email protected]>: > Yes, it would be good to refactor that, adding a base class sounds good. > > Jörn > > On Tue, Mar 14, 2017 at 11:08 PM, Peter Thygesen < > [email protected] > > wrote: > > > 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 > > > > > >
