macOS Point out a Pointer Mistake

[ArtOfWarfare]

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.

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:

- (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?

 

[HexMonkey]

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?

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.

 

[dylanryan]

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.

 

[chown33]

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.

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.

 

[HexMonkey]

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.

 

[ArtOfWarfare]

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.

double averageIn(double newNumber, double existingAverage, long existingCount) {
    double divisor = existingCount + 1.0;
    double weightedExistingAverage = existingAverage * existingCount / divisor;
    return newNumber / divisor + weightedExistingAverage;
}

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);
}

 

[chown33]

This code can be simplified:

double averageIn(double newNumber, double existingAverage, long existingCount) {
    double divisor = existingCount + 1.0;
    double weightedExistingAverage = existingAverage * existingCount / divisor;
    return newNumber / divisor + weightedExistingAverage;
}

to this:

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.

 

[ArtOfWarfare]

This code can be simplified:

double averageIn(double newNumber, double existingAverage, long existingCount) {
    double divisor = existingCount + 1.0;
    double weightedExistingAverage = existingAverage * existingCount / divisor;
    return newNumber / divisor + weightedExistingAverage;
}

to this:

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.

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?

 

[gnasher729]

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?

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 ), so you can average any numbers up to 10^290.

But you could also write

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.

 

[ArtOfWarfare]

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.

 

[chown33]

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.

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.