APSIMInitiative / ApsimX

ApsimX is the next generation of APSIM
http://www.apsim.info
Other
136 stars 164 forks source link

The function 'MathUtilities.Sum' does not include the last index #3372

Closed BrianCollinss closed 5 years ago

BrianCollinss commented 5 years ago

In the function MathUtilities.Sum, last index is never included in the summation. When iIndex reaches iEndIndex, the result is returned without accounting for the last value.

public static double Sum(IEnumerable Values, int iStartIndex, int iEndIndex,
                                double dInitialValue)
        {
            double result = dInitialValue;
            if (iStartIndex < 0)
                throw new Exception("MathUtilities.Sum: End index or start index is out of range");
            int iIndex = 0;
            foreach (double Value in Values)
            {
                if (iIndex >= iEndIndex)
                    return result;
                if (iIndex >= iStartIndex && Value != MissingValue)
                    result += Value;
                iIndex++;
            }
            return result;
        }
rcichota commented 5 years ago

@BenAbabaei , wondering why did you close this issue, I just came across the same problem (or I am interpreting something wrong?)

her123 commented 5 years ago

It appears that this function is only every used in relation to soil layers. It should be named for that use and not named in generic terms that lead to confusion.

BrianCollinss commented 5 years ago

Dean told me that it was intentional. Though I still think it should be changed, I left it to others to decide what is best. It's only used 3-4 times in the code. But it's a bad idea to name it Sum as the other Sum function doesn't behave the same way. Also, if one is to ignore the last layer, they can simply do it by changing the EndIndex. One can easily use it in a wrong way. You can see my other issue about auto-Irrigation manager (https://github.com/APSIMInitiative/ApsimX/issues/3373) in which the same function leads to a wrong result.

rcichota commented 5 years ago

Thanks @BenAbabaei and @her123 I believe this may be code from Classic (array with base 1?). I think we could create a more generic Sum that could be used for any array. This can be a useful function in manager scripts...

BrianCollinss commented 5 years ago

Like I said, as my suggestion was rejected once, I'd rather leave it to you guys and @hol430. Cheers.

rcichota commented 5 years ago

Hi @BenAbabaei I have added an override Sum() function that should do what you (and I) wanted. It takes only three parameters: the array, the initial index, and the final index over which the sum is performed - both inclusively) This do not change any use of the older version, so the irrigation script you mentioned still has the problem of not considering the correct layers, but it could be fixed using the new version of the function...

BrianCollinss commented 5 years ago

Thanks @rcichota . I did the same thing a while ago but my commit wasn't merged by the admins. I hope yours is. Cheers. B.

hol430 commented 5 years ago

@rcichota has your modified Sum() function been merged?

rcichota commented 5 years ago

Hi @BenAbabaei just confirming that the alternative Sum() method has been merged.

hol430 commented 5 years ago

Ah ok. @BenAbabaei see APSIMInitiative/APSIM.Shared#75 for details. Closing this issue now.