commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Artem Barger (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (MATH-1372) Add argmin/argmax static functions to MathArrays utility class
Date Wed, 01 Jun 2016 13:45:59 GMT

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

Artem Barger commented on MATH-1372:
------------------------------------

Hi,

First of all thanks for reviewing my code and writing detailed explanation.

I'm wondering what is the benefit of private static method, which basically 
repeats logic of methods I've suggested and has an additional boolean parameter
which indicate whenever we would like to get min or max elements index.


While I think it terms or reducing code duplication following could have more 
sense:
{code}
    private static int argMinOrMax(final double[] data,
                                   boolean isMin) {
        if (data == null ||
                data.length == 0) {
            return -1;
        }

        int index = 0;
        int direction = (isMin) ? 1 : -1;
        double selected = direction * data[0];

        for (int i = 1; i < data.length; i++) {
            final double current = direction * data[i];
            if (current < selected) {
                index = i;
                selected = current;
            }
        }
        return index;
    }
{code}

at least now it avoids code duplication at some extent.

Regarding preconditions,  I've took other method of {{MathArrays}} to look what is expected
behavior for precondition testing, but didn't find something specific hence assumed that getting
an NPE is expected. Obviously I was wrong, but that also mean methods such as: {{unique}},
{{shuffle}}, {{linearCombination}} and probably a couple of other also need to be fixed or
changed to take care of nulls and other possible incorrect inputs.


I'm sorry for the imports part, I've forgot to turn off this annoying feature of my IDEA.


And thanks again for catching a BUG in the tests w/ Double.MIN_VALUE.


With regards to the right place, I'm not really sure, from one point of view it make sense
to keep it here, since {{MathArray}} includes a lot of utilities to work with arrays, but
I've discovered that {{StatUtils}} also includes relevant parts as for example finding minimum
or maximum value in the arrays. 

> Add argmin/argmax static functions to MathArrays utility class
> --------------------------------------------------------------
>
>                 Key: MATH-1372
>                 URL: https://issues.apache.org/jira/browse/MATH-1372
>             Project: Commons Math
>          Issue Type: Wish
>            Reporter: Artem Barger
>            Assignee: Artem Barger
>            Priority: Trivial
>         Attachments: MATH-1372.patch
>
>
> Following conversation in the ML thread.
> Working lately w/ CM, I've found myself using a lot functions which helps to identify
the index of the min/max elements in given array of doubles. 
> In ML it was pointed out that similar functionality already exists in RealVector, however
due to MATH-765 it's planned to be removed (deprecated), moreover to use it one has to create
an instance of RealVector, where for simple cases it might be redundant.
> Hence I'd like to propose to add these static methods to the MathArrays utility class.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message