You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@opennlp.apache.org by "william.colen@gmail.com" <wi...@gmail.com> on 2011/08/19 12:38:54 UTC
Re: svn commit: r1159447 - in /incubator/opennlp/trunk/opennlp-tools/src:
main/java/opennlp/tools/chunker/ main/java/opennlp/tools/cmdline/chunker/
main/java/opennlp/tools/cmdline/namefind/ main/java/opennlp/tools/cmdline/postag/
main/java/opennlp/to
On Fri, Aug 19, 2011 at 6:11 AM, Jörn Kottmann <ko...@gmail.com> wrote:
> On 8/19/11 1:40 AM, colen@apache.org wrote:
>
>> ==============================**==============================**
>> ==================
>> --- incubator/opennlp/trunk/**opennlp-tools/src/main/java/**
>> opennlp/tools/util/eval/**Evaluator.java (original)
>> +++ incubator/opennlp/trunk/**opennlp-tools/src/main/java/**
>> opennlp/tools/util/eval/**Evaluator.java Thu Aug 18 23:40:24 2011
>> @@ -19,7 +19,6 @@
>> package opennlp.tools.util.eval;
>>
>> import java.io.IOException;
>> -import java.util.LinkedList;
>> import java.util.List;
>>
>> import opennlp.tools.util.**ObjectStream;
>> @@ -32,7 +31,15 @@ import opennlp.tools.util.**ObjectStream;
>> */
>> public abstract class Evaluator<T> {
>>
>> - private List<EvaluationSampleListener<**T>> listeners = new
>> LinkedList<**EvaluationSampleListener<T>>()**;
>> + private List<EvaluationSampleListener<**T>> listeners;
>> +
>> + public Evaluator() {
>> + this.listeners = null;
>> + }
>> +
>> + public Evaluator(List<**EvaluationSampleListener<T>> listeners) {
>> + this.listeners = listeners;
>> + }
>>
>
> I still believe that the case where you just have one
> EvaluationSampleListener object that
> should be hooked up is the most frequent use case, and its very rare that
> people want to
> hookup multiple.
>
> To make that easy, I suggest that we add a constructor which just takes one
> listener.
>
Should we do it to all XYEvaluators or only with the Evaluator class? It is
a lot of new constructors!
And I now more and more feel like that we picked a bad name with
> EvaluationSampleListener,
> sounds a little confusing. Maybe EvaluationMonitor would be better, any
> opinions?
>
Yes, sounds better. I will change if after the other refactorings.
Re: svn commit: r1159447 - in /incubator/opennlp/trunk/opennlp-tools/src:
main/java/opennlp/tools/chunker/ main/java/opennlp/tools/cmdline/chunker/
main/java/opennlp/tools/cmdline/namefind/ main/java/opennlp/tools/cmdline/postag/
main/java/opennlp/to
Posted by "william.colen@gmail.com" <wi...@gmail.com>.
On Fri, Aug 19, 2011 at 7:55 AM, Jörn Kottmann <ko...@gmail.com> wrote:
> On 8/19/11 12:38 PM, william.colen@gmail.com wrote:
>
>> > I still believe that the case where you just have one
>>> > EvaluationSampleListener object that
>>> > should be hooked up is the most frequent use case, and its very rare
>>> that
>>> > people want to
>>> > hookup multiple.
>>> >
>>> > To make that easy, I suggest that we add a constructor which just
>>> takes one
>>> > listener.
>>> >
>>>
>> Should we do it to all XYEvaluators or only with the Evaluator class? It
>> is
>> a lot of new constructors!
>>
>
> Maybe we can do it only in the XYEvaluators, and make it call the one with
> the list,
> we can use the Collections util to create the list, like you did in one of
> your tests.
>
> The old constructor should also call the List constructor, to avoid code
> duplication.
>
> I can help out here, if you want.
>
> Jörn
>
Thank you, Jörn. I will work on that tomorrow (Saturday).
Re: svn commit: r1159447 - in /incubator/opennlp/trunk/opennlp-tools/src:
main/java/opennlp/tools/chunker/ main/java/opennlp/tools/cmdline/chunker/
main/java/opennlp/tools/cmdline/namefind/ main/java/opennlp/tools/cmdline/postag/
main/java/opennlp/to
Posted by Jörn Kottmann <ko...@gmail.com>.
On 8/19/11 12:38 PM, william.colen@gmail.com wrote:
>> > I still believe that the case where you just have one
>> > EvaluationSampleListener object that
>> > should be hooked up is the most frequent use case, and its very rare that
>> > people want to
>> > hookup multiple.
>> >
>> > To make that easy, I suggest that we add a constructor which just takes one
>> > listener.
>> >
> Should we do it to all XYEvaluators or only with the Evaluator class? It is
> a lot of new constructors!
Maybe we can do it only in the XYEvaluators, and make it call the one
with the list,
we can use the Collections util to create the list, like you did in one
of your tests.
The old constructor should also call the List constructor, to avoid code
duplication.
I can help out here, if you want.
Jörn
Re: svn commit: r1159447 - in /incubator/opennlp/trunk/opennlp-tools/src:
main/java/opennlp/tools/chunker/ main/java/opennlp/tools/cmdline/chunker/
main/java/opennlp/tools/cmdline/namefind/ main/java/opennlp/tools/cmdline/postag/
main/java/opennlp/to
Posted by "william.colen@gmail.com" <wi...@gmail.com>.
On Tue, Aug 23, 2011 at 10:41 AM, Jörn Kottmann <ko...@gmail.com> wrote:
> On 8/23/11 3:40 PM, william.colen@gmail.com wrote:
>
>> On Tue, Aug 23, 2011 at 5:31 AM, Jörn Kottmann<ko...@gmail.com>
>> wrote:
>>
>> On 8/23/11 6:07 AM, william.colen@gmail.com wrote:
>>>
>>> I added variable length param only for the Chunker. Please check if it
>>>> looks
>>>> OK. If yes I will do it for the other tools.
>>>>
>>>> Looks good to me, you don't need the null check for the variable length
>>> param. If the user passes null, he gets a NullPointerException.
>>>
>>> In the case he doesn't pass anything you get an empty array.
>>>
>>> What do you think, is the variable length param way better than the
>>> other?
>>>
>>> Yes, it looks nicer. I will do it for the other tools now.
>>
>> +1
>
> Jörn
>
I finished it. If you agree with the changes I will close the issue.
Re: svn commit: r1159447 - in /incubator/opennlp/trunk/opennlp-tools/src:
main/java/opennlp/tools/chunker/ main/java/opennlp/tools/cmdline/chunker/
main/java/opennlp/tools/cmdline/namefind/ main/java/opennlp/tools/cmdline/postag/
main/java/opennlp/to
Posted by Jörn Kottmann <ko...@gmail.com>.
On 8/23/11 3:40 PM, william.colen@gmail.com wrote:
> On Tue, Aug 23, 2011 at 5:31 AM, Jörn Kottmann<ko...@gmail.com> wrote:
>
>> On 8/23/11 6:07 AM, william.colen@gmail.com wrote:
>>
>>> I added variable length param only for the Chunker. Please check if it
>>> looks
>>> OK. If yes I will do it for the other tools.
>>>
>> Looks good to me, you don't need the null check for the variable length
>> param. If the user passes null, he gets a NullPointerException.
>>
>> In the case he doesn't pass anything you get an empty array.
>>
>> What do you think, is the variable length param way better than the other?
>>
> Yes, it looks nicer. I will do it for the other tools now.
>
+1
Jörn
Re: svn commit: r1159447 - in /incubator/opennlp/trunk/opennlp-tools/src:
main/java/opennlp/tools/chunker/ main/java/opennlp/tools/cmdline/chunker/
main/java/opennlp/tools/cmdline/namefind/ main/java/opennlp/tools/cmdline/postag/
main/java/opennlp/to
Posted by "william.colen@gmail.com" <wi...@gmail.com>.
On Tue, Aug 23, 2011 at 5:31 AM, Jörn Kottmann <ko...@gmail.com> wrote:
> On 8/23/11 6:07 AM, william.colen@gmail.com wrote:
>
>> I added variable length param only for the Chunker. Please check if it
>> looks
>> OK. If yes I will do it for the other tools.
>>
> Looks good to me, you don't need the null check for the variable length
> param. If the user passes null, he gets a NullPointerException.
>
> In the case he doesn't pass anything you get an empty array.
>
> What do you think, is the variable length param way better than the other?
>
Yes, it looks nicer. I will do it for the other tools now.
Re: svn commit: r1159447 - in /incubator/opennlp/trunk/opennlp-tools/src:
main/java/opennlp/tools/chunker/ main/java/opennlp/tools/cmdline/chunker/
main/java/opennlp/tools/cmdline/namefind/ main/java/opennlp/tools/cmdline/postag/
main/java/opennlp/to
Posted by Jörn Kottmann <ko...@gmail.com>.
On 8/23/11 6:07 AM, william.colen@gmail.com wrote:
> I added variable length param only for the Chunker. Please check if it looks
> OK. If yes I will do it for the other tools.
Looks good to me, you don't need the null check for the variable length
param. If the user passes null, he gets a NullPointerException.
In the case he doesn't pass anything you get an empty array.
What do you think, is the variable length param way better than the other?
Jörn
Re: svn commit: r1159447 - in /incubator/opennlp/trunk/opennlp-tools/src:
main/java/opennlp/tools/chunker/ main/java/opennlp/tools/cmdline/chunker/
main/java/opennlp/tools/cmdline/namefind/ main/java/opennlp/tools/cmdline/postag/
main/java/opennlp/to
Posted by "william.colen@gmail.com" <wi...@gmail.com>.
On Mon, Aug 22, 2011 at 8:46 AM, Jörn Kottmann <ko...@gmail.com> wrote:
> On 8/19/11 12:38 PM, william.colen@gmail.com wrote:
>
>> > I still believe that the case where you just have one
>>> > EvaluationSampleListener object that
>>> > should be hooked up is the most frequent use case, and its very rare
>>> that
>>> > people want to
>>> > hookup multiple.
>>> >
>>> > To make that easy, I suggest that we add a constructor which just
>>> takes one
>>> > listener.
>>> >
>>>
>> Should we do it to all XYEvaluators or only with the Evaluator class? It
>> is
>> a lot of new constructors!
>>
>
> I saw your changes now. With a variable length param it would look much
> nicer,
> because then we would only need on constructor, instead of three.
>
> The reason why we don't use them here was that we cannot create an array of
> a parametrized
> type in java, anyway it would be possible but then it is not type-safe and
> we get a compiler
> warning.
>
> To make it type safe we would need to define an interface like this one:
> TokenizerEvaluationMonitor implements EvaluationMonitor<TokenSample>
>
> If we use such an interface we loos a bit of flexibility, in the end it is
> a trade off
> between using variable length params in our constructors, or to have this
> little
> bit more flexibility.
>
> Variable length parms in our constructors, might also be slightly more
> convenient to use.
>
> Jörn
>
Jörn,
I added variable length param only for the Chunker. Please check if it looks
OK. If yes I will do it for the other tools.
William
Re: svn commit: r1159447 - in /incubator/opennlp/trunk/opennlp-tools/src:
main/java/opennlp/tools/chunker/ main/java/opennlp/tools/cmdline/chunker/
main/java/opennlp/tools/cmdline/namefind/ main/java/opennlp/tools/cmdline/postag/
main/java/opennlp/to
Posted by Jörn Kottmann <ko...@gmail.com>.
On 8/19/11 12:38 PM, william.colen@gmail.com wrote:
>> > I still believe that the case where you just have one
>> > EvaluationSampleListener object that
>> > should be hooked up is the most frequent use case, and its very rare that
>> > people want to
>> > hookup multiple.
>> >
>> > To make that easy, I suggest that we add a constructor which just takes one
>> > listener.
>> >
> Should we do it to all XYEvaluators or only with the Evaluator class? It is
> a lot of new constructors!
I saw your changes now. With a variable length param it would look much
nicer,
because then we would only need on constructor, instead of three.
The reason why we don't use them here was that we cannot create an array
of a parametrized
type in java, anyway it would be possible but then it is not type-safe
and we get a compiler
warning.
To make it type safe we would need to define an interface like this one:
TokenizerEvaluationMonitor implements EvaluationMonitor<TokenSample>
If we use such an interface we loos a bit of flexibility, in the end it
is a trade off
between using variable length params in our constructors, or to have
this little
bit more flexibility.
Variable length parms in our constructors, might also be slightly more
convenient to use.
Jörn