6

I have a simple function template to calculate the average value of a container:

template<typename T>
T array_average( std::vector<T>& values ) {
    if( std::is_arithmetic<T>::value ) {
        if( !values.empty() ) {
            if( values.size() == 1 ) {
                return values[0];
            } else { 
                return (static_cast<T>( std::accumulate( values.begin(), values.end(), 0 )  ) / static_cast<T>( values.size() ) );
            }
        } else {
            throw std::runtime_error( "Can not take average of an empty container" ); 
        }
    } else {
        throw std::runtime_error( "T is not of an arithmetic type" );
    }
}

I added in the static_cast<>s above to try to force the calculation to the desired type <T>.

When I call this function in main using an uint64_t

std::vector<uint64_t> values{ 1,2,3,4,5,6,7,8,9,10,11,12 };
std::cout << array_average( values ) << '\n';

This codes does produce MSVC's compiler warning C4244 possible loss of data due to conversion, but it runs properly and this gives me the expected result and it prints out 6 to the console. This is correct as the actual value is 6.5 but due to the truncation in integer division 6 is correct.

Now If I use the function above with this instead:

std::vector<double> values { 2.0, 3.5, 4.5, 6.7, 8.9 };
std::cout << array_average( values2 ) << '\n';

This should give me a result of 5.12 however it is displaying 4.6 instead. This also gives me the same compiler warning as above, but it runs without a runtime error (break in execution) but is giving me incorrect results.

I'm not sure where the bug is in my function. I don't know if this is due to that compiler warning or not, or if it's the way I designed the function itself.

-Edit-

A user has suggested that this may be a duplicate of this Q/A I can not argue against the fact that it does or does not answer my question. At the time of asking this question; I did not know that the bug was coming from the improper use of std::accumulate itself. I wasn't sure if it was coming from the compiler warning that pertained to possible loss of data due to conversion, or if I was casting it wrong, or if it was in how I implemented this function in general. I had already accepted the answer that is found on this page before the link was provided. I will leave this Q/A as is for future reference and readers! Other than that I do appreciate the provided link as it does help to understand where the error was located in my code, what the error was and what was causing it, and how to properly fix it in addition to the accepted answer on this page.

Francis Cugler
  • 7,788
  • 2
  • 28
  • 59
  • 3
    It should be returning 5.12, not 6.4. – jwimberley Nov 07 '18 at 13:04
  • `throw std::runtime_error( "T is not of an arithmetic type" );` this one should be a compile error instead of a runtime exception. – llllllllll Nov 07 '18 at 13:05
  • @liliscent It will be. – Angew is no longer proud of SO Nov 07 '18 at 13:05
  • @jwimberley I'll have to check the numbers of both arrays in my ide and here to see if I made a typo somewhere, and to run them through the calc again to see if the result is right and wrong. – Francis Cugler Nov 07 '18 at 13:07
  • 1
    @jwimberley you are correct; I must of added the values into windows calc wrong... it is 5.12 and not 6.4. but the function was still producing incorrect values. – Francis Cugler Nov 07 '18 at 13:09
  • 2
    Use less arbitrary test cases – start with ones where it's easy to tell that they're wrong. `{1.5, 1.5}` is enough to expose the bug, and if you didn't have the unnecessary special case for a one-element input, `{1.5}` would suffice. – molbdnilo Nov 07 '18 at 13:17
  • @molbdnilo About the `special case for one element`: this is to return that value if there is only a single element in the container this way the function doesn't call `std::accumulate` and divide by the container's size! It's for performance reasons. – Francis Cugler Nov 07 '18 at 13:22
  • 3
    @FrancisCugler I know what it's for. Unless you know that the vast majority of averaging will be of single-element containers and you've measured the performance and determined that it matters, it's a pointless premature optimisation that only adds complexity. – molbdnilo Nov 07 '18 at 13:25
  • @molbdnilo Even with a majority of averaging done on a single element container, I'd bet the perf would be equal. – YSC Nov 07 '18 at 13:29
  • 1
    The third argument of `std::accumulate()` needs to be of type `T` (given that you want a result of type `T`) but you are supplying an `int` literal. Fixing that would negate any need for most of the type conversions. Also, bear in mind that if `T` is integral, both the total and average will be calculated as integral values (with rounding toward zero). – Peter Nov 07 '18 at 13:37
  • 2
    The fact that your question is closed as a duplicate is not judgmental. The condition for it to be closed as such is _"this question has already be answered here"_. It's true you couldn't guess what to look for to find this answer; you did well by asking this question (this is a good question IMO). Nevertheless, there is an answer to your question in the linked duplicate. Have a good day. – YSC Nov 08 '18 at 12:21
  • @AndyG You can find it here: https://stackoverflow.com/q/54155349/1757805 – Francis Cugler Jan 11 '19 at 23:32
  • @AndyG was having issues with web browser, so I just started a new question... – Francis Cugler Jan 11 '19 at 23:32

1 Answers1

18

Your static_cast is in the wrong place. You're casting the result of the accumulation, but letting the accumulation run in the type of the initial term (here 0, which is int). So do this instead:

return std::accumulate( values.begin(), values.end(), static_cast<T>(0) ) / static_cast<T>( values.size() );

(Note that 4.6 is indeed the result of static_cast<double>(2 + 3 + 4 + 6 + 8) / 5.0).

Comments unrelated to the core of the question:

  • The function should be taking const std::vector<T>&, because it doesn't modify values.
  • If you call the function with a T which is not valid for std::accumulate (e.g. not arithmetic), you will get a compile-time error. The topmost if would have to be if constexpr to work the way you want it to.
Angew is no longer proud of SO
  • 167,307
  • 17
  • 350
  • 455
  • Sounds good, now with the `if constexpr` on the failed case: should I be throwing `std::runtime_error(...)` like I am or should I use: `static_assert(dependent_false::value, "Must be arithmetic");` instead? What's the major differences between the two... – Francis Cugler Nov 07 '18 at 13:19
  • @FrancisCugler It depends on what you want to achieve. If you want a compile-time error, do the `static_assert` (and you can get rid of that `if` altogether). If you actually want a runtime error, throw one. – Angew is no longer proud of SO Nov 07 '18 at 13:20
  • 1
    @FrancisCugler `static_assert` is evaluated at compile time, while `throw` is evaluated at runtime. Presumably you would want to check as many things as possible, at compile time. – Algirdas Preidžius Nov 07 '18 at 13:21
  • 1
    Another comment unrelated to the core of the question: Better than using the `std::is_arithmetic` trait, you should write a custom trait (or a concept) to check than `T` provides a `+` and a `/`. This way, a type mimicking an arithmetic type (e.g. `std::complex`) could be used. – YSC Nov 07 '18 at 13:26
  • couldn't `static_cast(0)` just be `T()`? – Alan Birtles Nov 07 '18 at 13:45
  • 1
    As usual with `std::accumulate`, `static_cast(0)` is also questionable, average of `/*unsigned*/ char` (with overflow) for example. – Jarod42 Nov 07 '18 at 13:59
  • Ad `std::is_arithmetic`, wouldn't be more explicit to use SFINAE/`enable_if` rather than `constexpr` or the runtime check? – wondra Nov 07 '18 at 14:57
  • 1
    @wondra It depends on what you're after. If you have an alternative overload for non-arithmetic types, or want to support traits such as "can `array_average` be called with this type?", then use SFINAE. Otherwise, I'd much prefer `static_assert` for its simplicity and *error message readability.* And if you need the check to happen at runtime (unlikely, but possible, e.g. because it's part of some evaluation of user-specified expressions), use a runtime error. – Angew is no longer proud of SO Nov 07 '18 at 15:20