Hi. 2019-08-19 17:10 UTC+02:00, Alex Herbert <alex.d.herb...@gmail.com>: > >> On 19 Aug 2019, at 15:45, akash srivastava <akashsp...@gmail.com> 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 <akashsp...@gmail.com> >> 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: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org