Point out a Pointer Mistake

Discussion in 'Mac Programming' started by ArtOfWarfare, Oct 11, 2013.

  1. ArtOfWarfare, Oct 11, 2013
    Last edited: Oct 11, 2013

    ArtOfWarfare macrumors 604

    ArtOfWarfare

    Joined:
    Nov 26, 2007
    #1
    I've been frolicking in pointerless languages too much recently it seems, as I can't get this simple C function (it's within an Obj-C project, thus the use of Core Foundation classes) to work as I want it to...

    This function is supposed to take an existing average value and a new value to average in, along with a weight for the existing value (the new value is assumed to just have a weight of 1.) It doesn't return a new value, but instead it should modify the existing average value that's passed in to instead hold a new average value.

    Code:
    void averageIn(NSNumber *newNumber, NSNumber *existingAverage,
                   NSNumber *existingCount)
    {
        double existingCountDouble = [existingCount doubleValue];
        double divisor = existingCountDouble + 1.0;
        double existingAverageWeight = existingCountDouble / divisor;
        double weightedExistingAverage = [existingAverage doubleValue]
                                        * existingAverageWeight;
        double weightedNewNumber = [newNumber doubleValue] / divisor;
        existingAverage = @(weightedExistingAverage + weightedNewNumber);
        NSLog(@"%@", existingAverage);
    }
    I wrote a unit test for this, which is demonstrating that something isn't working for me:
    Code:
    - (void)testAverageIn
    {
        NSNumber *existingCount = @1;
        NSNumber *existingAverage = @5.0;
        averageIn(@2.0, existingAverage, existingCount);
        XCTAssertEqualWithAccuracy(existingAverage.doubleValue, 3.5, 0.1, @"Messed up averaging...");
    }
    The NSLog statement inside the method is printing out 3.5, which tells me that the arithmetic is right (or it at least happens to pass this test case... I'll write a few more to make sure it's not a fluke,) but it's failing this test and saying that 5 is not equal to 3.5 +/- 0.1, which tells me the existing average isn't actually being updated to the value that the NSLog statement prints out.

    It seems obvious to me that I've made a mistake with pointers here somewhere, but I'm not quite sure where... I'm wondering if maybe I should be working with double pointers instead?
     
  2. HexMonkey Administrator

    HexMonkey

    Staff Member

    Joined:
    Feb 5, 2004
    Location:
    New Zealand
    #2
    Yup, you'll need a double pointer for existingAverage.

    Currently, when you call averageIn, it makes a copy of the existingAverage pointer - so they both point to the original number, but the pointers themselves are at different locations in memory. When you reassign existingAverage in averageIn, it creates a new object that the averageIn version of existingAverage points to, but the testAverageIn version of existingAverage still points to the old number. I've attached a diagram to illustrate this.

    To fix it, change the existingAverage parameter to a double pointer (NSNumber **), change the call to "averageIn(@2.0, &existingAverage, existingCount);", and dereference existingAverage when you use it within averageIn (ie, change existingAverage to *existingAverage). This will ensure that you're working with the same pointer in both functions.
     

    Attached Files:

  3. dylanryan macrumors member

    Joined:
    Aug 21, 2011
    #3
    From what I understand of Obejctive C (which isn't much), you always use NSNumber*, not NSNumber even in local functions, right? I.e. if you just wanted to use a NSNumber inside a function, you'd use a NSNumber*

    If so, then if you want to have a pointer to (A NSNumber pointer), you need to pass in a NSNumber**, and then do *existingAverage=@(...);. Right now, you are just changing the value of the local pointer, not the original.


    if you used a double, you could get away with just passing in a double*.

    Could be way off base. But basically, you need one more * than you'd "normally" use if you want to make this work.
     
  4. chown33, Oct 11, 2013
    Last edited by a moderator: Oct 11, 2013

    chown33 macrumors 604

    Joined:
    Aug 9, 2009
    #4
    An NSNumber object is immutable. So the underlined part is impossible to achieve.

    You can, however, create a new NSNumber object with the new result, but then you'd have to do one of the following:
    1. Return the NSNumber* as the return value of the function.
    2. Pass a suitable reference-type as the existingAverage parameter. As already noted, the correct type would be NSNumber **.

    Personally, I would refactor this. I'd write a function that took double arg types, and returned type double. This function would do the calculation, and not need to know nor care about NSNumber objects.

    Then I'd write a function that returned the NSNumber* type, and have it do all the conversions. It would also be responsible for making the new NSNumber* that held the result, and auto-releasing that object if appropriate. And this function's name would clearly indicate who owned the returned NSNumber object; i.e. it would follow the correct naming convention regarding returned objects.

    If there's a rationale for why the function needs to modify its existingAverage parameter, please post it. I see no obvious reason in anything posted so far, and I'm wondering if there's a real justifiable constraint there, or whether it's been artificially imposed for some unstated reason.
     
  5. HexMonkey Administrator

    HexMonkey

    Staff Member

    Joined:
    Feb 5, 2004
    Location:
    New Zealand
    #5
    I agree with chown33 that it should be refactored unless you have a really good reason for doing it the way you're currently doing it. The approach I (and dylanryan) outlined will work in your example with the test function, but may run into problems with more complex situations, such as if you're storing the number in an NSArray or other cases where pointers are copied between data structures. Plus, it doesn't lend itself to being the most readable/easy-to-maintain code. :)
     
  6. ArtOfWarfare, Oct 11, 2013
    Last edited: Oct 12, 2013

    ArtOfWarfare thread starter macrumors 604

    ArtOfWarfare

    Joined:
    Nov 26, 2007
    #6
    I wrote this code months ago and I didn't leave any comments explaining why I chose NSNumbers over doubles, so any reasoning has been completely forgotten. I doubt I knew that NSNumbers were immutable, though. I suppose that explains why they lack the kinds of methods you'd expect numbers to have (like, add, subtract, increment, and so on...)

    I'll probably leave a comment suggesting that if I have time in the future I should look into refactoring the method to use doubles instead... and a return value makes sense, too... I'm not sure why I opted to have it return nothing... I probably thought it would save memory or something.

    Even dumber - existingCount doesn't get modified by this function (I increment it in the methods that call the function, instead)... I'm suddenly wondering if I should actually change this into a little "Average" class that you hand numbers to, it keeps track of its own state, and you just ask it for the average back periodically. That would probably be a great idea for simplifying this code, given how many different "average" statistics I keep track of in the app.

    Edit: Alright, I figured out some of the reasoning behind my design choices... it's because I was taking the value out of an NSDictionary and sticking it right back in. But it's dumb, because all it causes is a lot of extra boxing and unboxing all over. I decided to change it so that in the same line that I access the value, I unbox it with [dictionary[key] doubleValue], and then when I set it I just wrap it back up in @(). The average function now just has two doubles an a long passed in, and it returns a new double.

    So I guess my solution ended up being just not to use pointers in a case where pointers really weren't needed and just degraded the readability of the code.

    Code:
    double averageIn(double newNumber, double existingAverage, long existingCount) {
        double divisor = existingCount + 1.0;
        double weightedExistingAverage = existingAverage * existingCount / divisor;
        return newNumber / divisor + weightedExistingAverage;
    }
    Code:
    const double HIGH_ACCURACY = 0.0001;
    
    - (void)testAverageIn {
        XCTAssertEqualWithAccuracy(averageIn(2.0, 5.0, 1), 3.5, HIGH_ACCURACY);
        XCTAssertEqualWithAccuracy(averageIn(2.5, 8.5, 1), 5.5, HIGH_ACCURACY);
        XCTAssertEqualWithAccuracy(averageIn(2.0, 5.0, 2), 4.0, HIGH_ACCURACY);
        XCTAssertEqualWithAccuracy(averageIn(2.5, 8.5, 2), 6.5, HIGH_ACCURACY);
    }
     
  7. chown33 macrumors 604

    Joined:
    Aug 9, 2009
    #7
    This code can be simplified:
    Code:
    double averageIn(double newNumber, double existingAverage, long existingCount) {
        double divisor = existingCount + 1.0;
        double weightedExistingAverage = existingAverage * existingCount / divisor;
        return newNumber / divisor + weightedExistingAverage;
    }
    
    to this:
    Code:
    double averageIn(double newNumber, double existingAverage, long existingCount) {
        double n = existingAverage * existingCount + newNumber;
        return n / (existingCount +1);
    }
    
    The simplified code has fewer divisions, and may have better accumulated-error characteristics, depending on what the relative magnitudes of the values being averaged actually are.

    For even better error characteristics, keep an actual total sum, and always calculate a new mean (average) from that. This avoids the problem of errors accumulating in the running mean.

    The may not be significant in this case, but often pays to know how errors accumulate due to order of calculation and the effects of precision.
     
  8. ArtOfWarfare thread starter macrumors 604

    ArtOfWarfare

    Joined:
    Nov 26, 2007
    #8
    Wouldn't your equation have overflow issues from existingAverage * existingCount before my equation has overflow issues? That's the reason I do the division separately - I figure it keeps the numbers smaller and so less likely to overflow.

    Edit: Would existingAverage * (existingCount / divisor) be more resilient than existingAverage * existingCount / divisor, since the division is done earlier on than otherwise?
     
  9. gnasher729, Oct 13, 2013
    Last edited: Oct 13, 2013

    gnasher729 macrumors P6

    gnasher729

    Joined:
    Nov 25, 2005
    #9
    How big are your numbers? "double" supports numbers up to something like 10^308. You are not averaging more than 10^18 numbers (because it takes so much time your Mac won't live long enough :D ), so you can average any numbers up to 10^290.

    But you could also write

    Code:
    newAverage = existingAverage + (newNumber - existingAverage) / (existingCount + 1);
    
    If you do this for a million numbers then rounding errors will creep up, but that's true for most methods. On a Mac, you can use long double for the calculation (that's what long double is there for). In general, the following method usually works well even if you need the average of gazillions of numbers:

    Keep a total of average and totalError. Initially both are 0.
    At each step, calculate the following:
    change = (newNumber - existingAverage) / (existingCount + 1)
    newAverage = existingAverage + change
    actualChange = newAverage - existingAverage
    totalError += (change - actualChange)
    existingAverage = newAverage
    At the very end, add totalError to existingAverage.
     
  10. ArtOfWarfare thread starter macrumors 604

    ArtOfWarfare

    Joined:
    Nov 26, 2007
    #10
    D'oh!

    I can't believe I made any argument about performance. Making code readable trumps all else. You shouldn't argue about performance unless it's been determined the code doesn't perform adequately.
     
  11. chown33 macrumors 604

    Joined:
    Aug 9, 2009
    #11
    Overflow isn't the same as precision loss.

    It almost seems like you're thinking of the values as long integers, where there is a danger of overflowing an intermediate calculated value. But that's not how floating-point works, and doubles are floating-point types, not integer types.
     

Share This Page