commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gilles (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (MATH-1019) "shuffle" method broken (in class "o.a.c.m.random.RandomDataGenerator")
Date Fri, 09 Aug 2013 21:24:49 GMT

    [ https://issues.apache.org/jira/browse/MATH-1019?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13735327#comment-13735327
] 

Gilles commented on MATH-1019:
------------------------------

IMHO, the svn log and the bug tracking system are not solely directed to library users.
Developers too might like to be able to follow why and how code changes.

I understand your concern, but "historically" MATH-1010 was not meant to fix something, just
to move code from one place to another; I fortuitously uncovered the bug because the method
was intended to do more than is actually needed in "RandomDataGenerator" ("user" behaviour
was indeed fine but "developer" code was misleading).

In order to not frighten users, I can add a comment in the "changes.xml" file to that effect;
i.e. for MATH-1020, something like: This bug does not affect applications using a previous
version of Commons Math. For this issue, it should be obvious since the method "shuffle" was
private.

                
> "shuffle" method broken (in class "o.a.c.m.random.RandomDataGenerator")
> -----------------------------------------------------------------------
>
>                 Key: MATH-1019
>                 URL: https://issues.apache.org/jira/browse/MATH-1019
>             Project: Commons Math
>          Issue Type: Bug
>    Affects Versions: 3.2
>            Reporter: Gilles
>             Fix For: 3.3
>
>
> The method does not abide by its contract: elements before the "end" index are included
in the shuffle.
> {code}
> /**
>  * Uses a 2-cycle permutation shuffle to randomly re-order the last elements 
>  * of list.
>  *
>  * @param list list to be shuffled
>  * @param end element past which shuffling begins
>  */
> private void shuffle(int[] list, int end) {
>     int target = 0;
>     for (int i = list.length - 1; i >= end; i--) {
>         if (i == 0) { // XXX "0" should be "end"
>             target = 0; // XXX "0" should be "end"
>         } else {
>             // NumberIsTooLargeException cannot occur
>             target = nextInt(0, i); // XXX "0" should be "end"
>         }
>         int temp = list[target];
>         list[target] = list[i];
>         list[i] = temp;
>     }
> }
> {code}
> I'm going to introduce the above corrections in the new implementation to be located
in "MathArrays" (cf. issue MATH-1010).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Mime
View raw message