Fix for the ArrayIndexOutOfBoundsException on calling NaturalRanking#rank() on an array of all NaNs

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Fix for the ArrayIndexOutOfBoundsException on calling NaturalRanking#rank() on an array of all NaNs

akash srivastava
Here is the link for the related bug:
https://issues.apache.org/jira/browse/MATH-1495

I have suggested two possible fixes when NaturalRanking#rank() is called on
an array of all NaNs:
1) throwing an IllegalArgumentException
2) Returning an empty array

I want to create a pull request regarding it, which fix do you suggest I
should go for?
Reply | Threaded
Open this post in threaded view
|

Re: Fix for the ArrayIndexOutOfBoundsException on calling NaturalRanking#rank() on an array of all NaNs

Gilles Sadowski-2
Hello.

Le jeu. 15 août 2019 à 11:33, akash srivastava <[hidden email]> a écrit :

>
> Here is the link for the related bug:
> https://issues.apache.org/jira/browse/MATH-1495
>
> I have suggested two possible fixes when NaturalRanking#rank() is called on
> an array of all NaNs:
> 1) throwing an IllegalArgumentException
> 2) Returning an empty array
>
> I want to create a pull request regarding it, which fix do you suggest I
> should go for?

There are rationale for each.  An exception would catch a programming bug early.
But returning an empty array would allow concise code when such a case is not a
bug.
WDYT?  Are there use-cases?

Gilles

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Fix for the ArrayIndexOutOfBoundsException on calling NaturalRanking#rank() on an array of all NaNs

Alex Herbert


> On 16 Aug 2019, at 16:06, Gilles Sadowski <[hidden email]> wrote:
>
> Hello.
>
> Le jeu. 15 août 2019 à 11:33, akash srivastava <[hidden email]> a écrit :
>>
>> Here is the link for the related bug:
>> https://issues.apache.org/jira/browse/MATH-1495
>>
>> I have suggested two possible fixes when NaturalRanking#rank() is called on
>> an array of all NaNs:
>> 1) throwing an IllegalArgumentException
>> 2) Returning an empty array
>>
>> I want to create a pull request regarding it, which fix do you suggest I
>> should go for?
>
> There are rationale for each.  An exception would catch a programming bug early.
> But returning an empty array would allow concise code when such a case is not a
> bug.
> WDYT?  Are there use-cases?

IIUC this is for the case when the NaNStrategy is REMOVED. So the result should match what happens when you call NaturalRanking.rank() with an empty array. Is that an empty array result or an exception?

>
> Gilles
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Fix for the ArrayIndexOutOfBoundsException on calling NaturalRanking#rank() on an array of all NaNs

Gilles Sadowski-2
Le ven. 16 août 2019 à 17:25, Alex Herbert <[hidden email]> a écrit :

>
>
>
> > On 16 Aug 2019, at 16:06, Gilles Sadowski <[hidden email]> wrote:
> >
> > Hello.
> >
> > Le jeu. 15 août 2019 à 11:33, akash srivastava <[hidden email]> a écrit :
> >>
> >> Here is the link for the related bug:
> >> https://issues.apache.org/jira/browse/MATH-1495
> >>
> >> I have suggested two possible fixes when NaturalRanking#rank() is called on
> >> an array of all NaNs:
> >> 1) throwing an IllegalArgumentException
> >> 2) Returning an empty array
> >>
> >> I want to create a pull request regarding it, which fix do you suggest I
> >> should go for?
> >
> > There are rationale for each.  An exception would catch a programming bug early.
> > But returning an empty array would allow concise code when such a case is not a
> > bug.
> > WDYT?  Are there use-cases?
>
> IIUC this is for the case when the NaNStrategy is REMOVED. So the result should match what happens when you call NaturalRanking.rank() with an empty array.

+1

> Is that an empty array result or an exception?

We could nevertheless change the behaviour (consistently) if one or
the other laternative is deemed better.

Regards,
Gilles

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Fix for the ArrayIndexOutOfBoundsException on calling NaturalRanking#rank() on an array of all NaNs

akash srivastava
In reply to this post by akash srivastava
I was advised to check with the dev mailing list if anyone has a problem
with the suggested fix.
It seems no one does have a problem with the fixes suggested so I'll go
forward and create a PR that returns an empty array for an array of all
NaNs.

On Thu, Aug 15, 2019 at 11:25 AM akash srivastava <[hidden email]>
wrote:

> Here is the link for the related bug:
> https://issues.apache.org/jira/browse/MATH-1495
>
> I have suggested two possible fixes when NaturalRanking#rank() is called
> on an array of all NaNs:
> 1) throwing an IllegalArgumentException
> 2) Returning an empty array
>
> I want to create a pull request regarding it, which fix do you suggest I
> should go for?
>
Reply | Threaded
Open this post in threaded view
|

Re: Fix for the ArrayIndexOutOfBoundsException on calling NaturalRanking#rank() on an array of all NaNs

Alex Herbert

> On 19 Aug 2019, at 15:45, akash srivastava <[hidden email]> wrote:
>
> I was advised to check with the dev mailing list if anyone has a problem
> with the suggested fix.
> It seems no one does have a problem with the fixes suggested so I'll go
> forward and create a PR that returns an empty array for an array of all
> NaNs.

I suggested doing the same as what happens when you pass in an empty array, effectively changing the array of NaNs to an array of nothing.

Reading the code it appears that this case would throw an ArrayIndexOutOfBoundsException. A simple test case shows that this is true, e.g. this passes:

@Test(expected=ArrayIndexOutOfBoundsException.class)
public void testEmpty() {
    new NaturalRanking().rank(new double[0]);
}

So the fix for rank() on an array of NaNs has no equivalent with a normal array as an empty normal array is an edge case currently not handled.

IMO returning an empty array is not going to be useful. If someone is using this class to rank data then immediately after ranking I would assume they will use the rank to do something, e.g. pick the top one. An empty array will fail for most use cases I can think of since they will require dereferencing the rank array. Only if the array is used without dereferencing it, e.g. print to file, or if the size of the rank array is checked before using the contents will an empty array be OK.

So the question becomes which is better:

- Returning an empty array and requiring users to check its length or else receive ArrayIndexOutOfBoundsException when using the array contents
- Throwing an exception so that the exact cause of their bug can be recorded in the stack trace

I would suggest that an array of NaNs or an empty array should throw the same and it should be some sort of IllegalArgumentException. Currently this class only throws NotANumberException for errors. Perhaps for these two cases an math4.exception.InsufficientDataException should be thrown. This basically states that you cannot rank nothing and any result that is returned will contain a ranking.

Opinions?

>
> On Thu, Aug 15, 2019 at 11:25 AM akash srivastava <[hidden email]>
> wrote:
>
>> Here is the link for the related bug:
>> https://issues.apache.org/jira/browse/MATH-1495
>>
>> I have suggested two possible fixes when NaturalRanking#rank() is called
>> on an array of all NaNs:
>> 1) throwing an IllegalArgumentException
>> 2) Returning an empty array
>>
>> I want to create a pull request regarding it, which fix do you suggest I
>> should go for?
>>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Fix for the ArrayIndexOutOfBoundsException on calling NaturalRanking#rank() on an array of all NaNs

Gilles Sadowski-2
Hi.

2019-08-19 17:10 UTC+02:00, Alex Herbert <[hidden email]>:

>
>> On 19 Aug 2019, at 15:45, akash srivastava <[hidden email]> wrote:
>>
>> I was advised to check with the dev mailing list if anyone has a problem
>> with the suggested fix.
>> It seems no one does have a problem with the fixes suggested so I'll go
>> forward and create a PR that returns an empty array for an array of all
>> NaNs.
>
> I suggested doing the same as what happens when you pass in an empty array,
> effectively changing the array of NaNs to an array of nothing.
>
> Reading the code it appears that this case would throw an
> ArrayIndexOutOfBoundsException. A simple test case shows that this is true,
> e.g. this passes:
>
> @Test(expected=ArrayIndexOutOfBoundsException.class)
> public void testEmpty() {
>     new NaturalRanking().rank(new double[0]);
> }
>
> So the fix for rank() on an array of NaNs has no equivalent with a normal
> array as an empty normal array is an edge case currently not handled.
>
> IMO returning an empty array is not going to be useful. If someone is using
> this class to rank data then immediately after ranking I would assume they
> will use the rank to do something, e.g. pick the top one. An empty array
> will fail for most use cases I can think of since they will require
> dereferencing the rank array. Only if the array is used without
> dereferencing it, e.g. print to file, or if the size of the rank array is
> checked before using the contents will an empty array be OK.
>
> So the question becomes which is better:
>
> - Returning an empty array and requiring users to check its length or else
> receive ArrayIndexOutOfBoundsException when using the array contents
> - Throwing an exception so that the exact cause of their bug can be recorded
> in the stack trace
>
> I would suggest that an array of NaNs or an empty array should throw the
> same and it should be some sort of IllegalArgumentException. Currently this
> class only throws NotANumberException for errors. Perhaps for these two
> cases an math4.exception.InsufficientDataException should be thrown. This
> basically states that you cannot rank nothing and any result that is
> returned will contain a ranking.
>
> Opinions?

It also seems to me that throwing an exception is more in line with
what has usually been done in CM (i.e. "better safe than sorry").

Best regards,
Gilles

>>
>> On Thu, Aug 15, 2019 at 11:25 AM akash srivastava <[hidden email]>
>> wrote:
>>
>>> Here is the link for the related bug:
>>> https://issues.apache.org/jira/browse/MATH-1495
>>>
>>> I have suggested two possible fixes when NaturalRanking#rank() is called
>>> on an array of all NaNs:
>>> 1) throwing an IllegalArgumentException
>>> 2) Returning an empty array
>>>
>>> I want to create a pull request regarding it, which fix do you suggest I
>>> should go for?
>>>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]