You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@opennlp.apache.org by Jörn Kottmann <ko...@gmail.com> on 2011/07/21 11:01:21 UTC
SentenceDetectorME.train API change
Hi,
the recent changes to SenenceDetectorME.train methods break backward
compatibility.
In the opennlp-uima package we have some code which calls the train method:
SentenceModel sentenceModel = SentenceDetectorME.train(language,
ObjectStreamUtils.createObjectStream(sentenceSamples), true, null);
Since it is passing in null for the dictionary the compiler now doesn't know
which of the two train methods should be called, and therefore produces
this error:
/home/hudson/hudson-slave/workspace/OpenNLP/trunk/opennlp-uima/src/main/java/opennlp/uima/sentdetect/SentenceDetectorTrainer.java:[137,52]
reference to train is ambiguous, both method
train(java.lang.String,opennlp.tools.util.ObjectStream<opennlp.tools.sentdetect.SentenceSample>,boolean,opennlp.tools.dictionary.Dictionary)
in opennlp.tools.sentdetect.SentenceDetectorME and method
train(java.lang.String,opennlp.tools.util.ObjectStream<opennlp.tools.sentdetect.SentenceSample>,boolean,opennlp.tools.dictionary.AbbreviationDictionary)
in opennlp.tools.sentdetect.SentenceDetectorME match
Since passing null is allowed, we cannot add such a method here.
I also noticed that you added the following method:
train(String languageCode,
ObjectStream<SentenceSample> samples, boolean useTokenEnd,
AbbreviationDictionary abbreviations, int cutoff, int iterations)
This should be removed again, because passing of cutoff and iterations
is replaced
by this new TrainingParameters object. We still have to go through the
code and deprecate
all these methods.
Jörn
Re: SentenceDetectorME.train API change
Posted by Jörn Kottmann <ko...@gmail.com>.
On 7/21/11 5:04 PM, Tommaso Teofili wrote:
> I don't have enough knowledge of this part but I wonder if a little
> refactoring consisting of creating an interface Dictionary and 2
> implementing classes i.e. SimpleDictionary and AbbreviationsDictionary could
> help.
Well, we are pretty limited here in terms of options because we only do
a minor release and cannot change our public API.
Adding a new type like we did here is ok, but the problem is how the train
method is overloaded.
Anyway we might end up doing some redesign of the the whole dictionary
support soon, to integrate other kinds of dictionaries, e.g. bloom
filter based
or one with a better internal data structure.
Since dictionaries are always shared it would be better if they are
immutable,
which they are currently not.
Jörn
Re: SentenceDetectorME.train API change
Posted by Tommaso Teofili <to...@gmail.com>.
2011/7/21 Jörn Kottmann <ko...@gmail.com>
> On 7/21/11 4:13 PM, william.colen@gmail.com wrote:
>
>> Should I just change the parameters order?
>>
>> For some reason the API was using a Dictionary to represent the
>> abbreviation
>> dictionary, but it was never used in the default context generator.
>> Initially I was thinking about using this Dictionary implementation, but
>> according to DefaultSDContextGenerator an abbreviation dictionary should
>> implement Set<String> and since Dictionary was already implementing
>> Iterable<StringList> it can't also implement Set<String>.
>>
>> Another option should be to remove the new AbbreviationDictionary class
>> and
>> try to use Dictionary instead. Maybe adding a method "asStringSet()" that
>> creates a Set<String> from the Dictionary and we can pass it to the
>> context
>> generator.
>>
>> What do you think?
>>
>
> The Dictionary is similar to the new Abbreviation Dictionary, but
> additionally supports
> storing entries which consist of multiple tokens.
>
> Do we have multi token abbreviations? If yes, we should use Dictionary.
> Otherwise we could still use it, then the tokenizer could have a small util
> method to turn a Dictionary into a Set<String>.
>
> Reusing the Dictionary makes a few things easier because we do not have
> to duplicate them.
>
> We can also change the DefaultSDContextGenerator, if that is more
> convenient.
>
>
I don't have enough knowledge of this part but I wonder if a little
refactoring consisting of creating an interface Dictionary and 2
implementing classes i.e. SimpleDictionary and AbbreviationsDictionary could
help.
My 0.0002 cents.
Tommaso
Re: SentenceDetectorME.train API change
Posted by Jörn Kottmann <ko...@gmail.com>.
On 7/21/11 7:33 PM, william.colen@gmail.com wrote:
> It is done. Now what should I do with OPENNLP-234 (Create Abbreviation
> Dictionary implementation and command line tools to build it)?
> Should I close it setting resolution to "won't fix", "invalid" or "later"?
Invalid or won't fix, sounds good, make sure to set the fix version to
empty.
Otherwise it will be included in the release notes as far as I know (but
maybe
I am mistaken).
Jörn
Re: SentenceDetectorME.train API change
Posted by "william.colen@gmail.com" <wi...@gmail.com>.
On Thu, Jul 21, 2011 at 12:41 PM, Jörn Kottmann <ko...@gmail.com> wrote:
> On 7/21/11 5:04 PM, william.colen@gmail.com wrote:
>
>> On Thu, Jul 21, 2011 at 11:38 AM, Jörn Kottmann<ko...@gmail.com>
>> wrote:
>>
>> On 7/21/11 4:13 PM, william.colen@gmail.com wrote:
>>>
>>> Should I just change the parameters order?
>>>>
>>>> For some reason the API was using a Dictionary to represent the
>>>> abbreviation
>>>> dictionary, but it was never used in the default context generator.
>>>> Initially I was thinking about using this Dictionary implementation, but
>>>> according to DefaultSDContextGenerator an abbreviation dictionary should
>>>> implement Set<String> and since Dictionary was already implementing
>>>> Iterable<StringList> it can't also implement Set<String>.
>>>>
>>>> Another option should be to remove the new AbbreviationDictionary class
>>>> and
>>>> try to use Dictionary instead. Maybe adding a method "asStringSet()"
>>>> that
>>>> creates a Set<String> from the Dictionary and we can pass it to the
>>>> context
>>>> generator.
>>>>
>>>> What do you think?
>>>>
>>>> The Dictionary is similar to the new Abbreviation Dictionary, but
>>> additionally supports
>>> storing entries which consist of multiple tokens.
>>>
>>> Do we have multi token abbreviations? If yes, we should use Dictionary.
>>> Otherwise we could still use it, then the tokenizer could have a small
>>> util
>>> method to turn a Dictionary into a Set<String>.
>>>
>>> Reusing the Dictionary makes a few things easier because we do not have
>>> to duplicate them.
>>>
>>> We can also change the DefaultSDContextGenerator, if that is more
>>> convenient.
>>>
>>>
>> OK, I'll revert my changes and do it using Dictionary.
>> I never had to do it in SVN, can you help me point me how to do it? I
>> searched the web how to do it but I'm not feeling confident. I'm using
>> Eclipse.
>> I need to revert the changes related to the issues 225 and 234.
>>
>>
>>
> You cannot undo/remove the commit, since we do not have admin
> rights on the subversion server.
>
> You can use the "Replace With" facility in eclipse to go back to the
> revision where it
> has been before you did the commit, then you maybe do you change and commit
> again.
> If you do that you must be careful not to undo other changes.
>
It is done. Now what should I do with OPENNLP-234 (Create Abbreviation
Dictionary implementation and command line tools to build it)?
Should I close it setting resolution to "won't fix", "invalid" or "later"?
I'll create a new issue to add a CL tool to create a Dictionary.
Re: SentenceDetectorME.train API change
Posted by Jörn Kottmann <ko...@gmail.com>.
On 7/21/11 5:04 PM, william.colen@gmail.com wrote:
> On Thu, Jul 21, 2011 at 11:38 AM, Jörn Kottmann<ko...@gmail.com> wrote:
>
>> On 7/21/11 4:13 PM, william.colen@gmail.com wrote:
>>
>>> Should I just change the parameters order?
>>>
>>> For some reason the API was using a Dictionary to represent the
>>> abbreviation
>>> dictionary, but it was never used in the default context generator.
>>> Initially I was thinking about using this Dictionary implementation, but
>>> according to DefaultSDContextGenerator an abbreviation dictionary should
>>> implement Set<String> and since Dictionary was already implementing
>>> Iterable<StringList> it can't also implement Set<String>.
>>>
>>> Another option should be to remove the new AbbreviationDictionary class
>>> and
>>> try to use Dictionary instead. Maybe adding a method "asStringSet()" that
>>> creates a Set<String> from the Dictionary and we can pass it to the
>>> context
>>> generator.
>>>
>>> What do you think?
>>>
>> The Dictionary is similar to the new Abbreviation Dictionary, but
>> additionally supports
>> storing entries which consist of multiple tokens.
>>
>> Do we have multi token abbreviations? If yes, we should use Dictionary.
>> Otherwise we could still use it, then the tokenizer could have a small util
>> method to turn a Dictionary into a Set<String>.
>>
>> Reusing the Dictionary makes a few things easier because we do not have
>> to duplicate them.
>>
>> We can also change the DefaultSDContextGenerator, if that is more
>> convenient.
>>
>
> OK, I'll revert my changes and do it using Dictionary.
> I never had to do it in SVN, can you help me point me how to do it? I
> searched the web how to do it but I'm not feeling confident. I'm using
> Eclipse.
> I need to revert the changes related to the issues 225 and 234.
>
>
You cannot undo/remove the commit, since we do not have admin
rights on the subversion server.
You can use the "Replace With" facility in eclipse to go back to the
revision where it
has been before you did the commit, then you maybe do you change and
commit again.
If you do that you must be careful not to undo other changes.
Jörn
Re: SentenceDetectorME.train API change
Posted by "william.colen@gmail.com" <wi...@gmail.com>.
On Thu, Jul 21, 2011 at 11:38 AM, Jörn Kottmann <ko...@gmail.com> wrote:
> On 7/21/11 4:13 PM, william.colen@gmail.com wrote:
>
>> Should I just change the parameters order?
>>
>> For some reason the API was using a Dictionary to represent the
>> abbreviation
>> dictionary, but it was never used in the default context generator.
>> Initially I was thinking about using this Dictionary implementation, but
>> according to DefaultSDContextGenerator an abbreviation dictionary should
>> implement Set<String> and since Dictionary was already implementing
>> Iterable<StringList> it can't also implement Set<String>.
>>
>> Another option should be to remove the new AbbreviationDictionary class
>> and
>> try to use Dictionary instead. Maybe adding a method "asStringSet()" that
>> creates a Set<String> from the Dictionary and we can pass it to the
>> context
>> generator.
>>
>> What do you think?
>>
>
> The Dictionary is similar to the new Abbreviation Dictionary, but
> additionally supports
> storing entries which consist of multiple tokens.
>
> Do we have multi token abbreviations? If yes, we should use Dictionary.
> Otherwise we could still use it, then the tokenizer could have a small util
> method to turn a Dictionary into a Set<String>.
>
> Reusing the Dictionary makes a few things easier because we do not have
> to duplicate them.
>
> We can also change the DefaultSDContextGenerator, if that is more
> convenient.
>
OK, I'll revert my changes and do it using Dictionary.
I never had to do it in SVN, can you help me point me how to do it? I
searched the web how to do it but I'm not feeling confident. I'm using
Eclipse.
I need to revert the changes related to the issues 225 and 234.
Thanks
William
Re: SentenceDetectorME.train API change
Posted by Jörn Kottmann <ko...@gmail.com>.
On 7/21/11 4:13 PM, william.colen@gmail.com wrote:
> Should I just change the parameters order?
>
> For some reason the API was using a Dictionary to represent the abbreviation
> dictionary, but it was never used in the default context generator.
> Initially I was thinking about using this Dictionary implementation, but
> according to DefaultSDContextGenerator an abbreviation dictionary should
> implement Set<String> and since Dictionary was already implementing
> Iterable<StringList> it can't also implement Set<String>.
>
> Another option should be to remove the new AbbreviationDictionary class and
> try to use Dictionary instead. Maybe adding a method "asStringSet()" that
> creates a Set<String> from the Dictionary and we can pass it to the context
> generator.
>
> What do you think?
The Dictionary is similar to the new Abbreviation Dictionary, but
additionally supports
storing entries which consist of multiple tokens.
Do we have multi token abbreviations? If yes, we should use Dictionary.
Otherwise we could still use it, then the tokenizer could have a small util
method to turn a Dictionary into a Set<String>.
Reusing the Dictionary makes a few things easier because we do not have
to duplicate them.
We can also change the DefaultSDContextGenerator, if that is more
convenient.
Jörn
Re: SentenceDetectorME.train API change
Posted by "william.colen@gmail.com" <wi...@gmail.com>.
On Thu, Jul 21, 2011 at 10:49 AM, Jörn Kottmann <ko...@gmail.com> wrote:
> On 7/21/11 3:46 PM, william.colen@gmail.com wrote:
>
>> Shouldn't we change UIMA sentence detector trainer to pass an abbreviation
>> dictionary? It would solve the issue for UIMA.
>>
>>
>> It won't help other people that is using the API and passing null. I will
>> think how to solve it.
>>
>
> No, the API must stay compatible with existing code.
Should I just change the parameters order?
For some reason the API was using a Dictionary to represent the abbreviation
dictionary, but it was never used in the default context generator.
Initially I was thinking about using this Dictionary implementation, but
according to DefaultSDContextGenerator an abbreviation dictionary should
implement Set<String> and since Dictionary was already implementing
Iterable<StringList> it can't also implement Set<String>.
Another option should be to remove the new AbbreviationDictionary class and
try to use Dictionary instead. Maybe adding a method "asStringSet()" that
creates a Set<String> from the Dictionary and we can pass it to the context
generator.
What do you think?
Re: SentenceDetectorME.train API change
Posted by Jörn Kottmann <ko...@gmail.com>.
On 7/21/11 3:46 PM, william.colen@gmail.com wrote:
> Shouldn't we change UIMA sentence detector trainer to pass an abbreviation
> dictionary? It would solve the issue for UIMA.
>
>
> It won't help other people that is using the API and passing null. I will
> think how to solve it.
No, the API must stay compatible with existing code.
Jörn
Re: SentenceDetectorME.train API change
Posted by "william.colen@gmail.com" <wi...@gmail.com>.
Shouldn't we change UIMA sentence detector trainer to pass an abbreviation
dictionary? It would solve the issue for UIMA.
It won't help other people that is using the API and passing null. I will
think how to solve it.
On Thu, Jul 21, 2011 at 6:01 AM, Jörn Kottmann <ko...@gmail.com> wrote:
> Hi,
>
> the recent changes to SenenceDetectorME.train methods break backward
> compatibility.
>
> In the opennlp-uima package we have some code which calls the train method:
> SentenceModel sentenceModel = SentenceDetectorME.train(**language,
> ObjectStreamUtils.**createObjectStream(**sentenceSamples), true,
> null);
>
> Since it is passing in null for the dictionary the compiler now doesn't
> know
> which of the two train methods should be called, and therefore produces
> this error:
> /home/hudson/hudson-slave/**workspace/OpenNLP/trunk/**
> opennlp-uima/src/main/java/**opennlp/uima/sentdetect/**
> SentenceDetectorTrainer.java:[**137,52] reference to train is ambiguous,
> both method train(java.lang.String,**opennlp.tools.util.**
> ObjectStream<opennlp.tools.**sentdetect.SentenceSample>,**
> boolean,opennlp.tools.**dictionary.Dictionary) in
> opennlp.tools.sentdetect.**SentenceDetectorME and method
> train(java.lang.String,**opennlp.tools.util.**ObjectStream<opennlp.tools.*
> *sentdetect.SentenceSample>,**boolean,opennlp.tools.**dictionary.**AbbreviationDictionary)
> in opennlp.tools.sentdetect.**SentenceDetectorME match
>
> Since passing null is allowed, we cannot add such a method here.
>
> I also noticed that you added the following method:
> train(String languageCode,
> ObjectStream<SentenceSample> samples, boolean useTokenEnd,
> AbbreviationDictionary abbreviations, int cutoff, int iterations)
>
> This should be removed again, because passing of cutoff and iterations is
> replaced
> by this new TrainingParameters object. We still have to go through the code
> and deprecate
> all these methods.
>
> Jörn
>
>
>