Mixing
![How to deal with a situation where a new team mate's experience makes him ramp up more easily and quickly as compared to yourself [closed]](https://blogger.googleusercontent.com/img/b/R29vZ2xl/AVvXsEgjbpfN9tAutmK93bJRC3ZoROZzi2TJDms5n8_qJuhgE0a9b52OOHayv3NGT8igAdFL7byXNst-_1DZK5SjrIJ28_6RQPUpBROqMs5s6jo-ZsjX8kjDwfxJufIitH3TaQRXWaGSQKRQib-f/s72-c/1.jpg)
Why is this function producing incorrect values?
Clash Royale CLAN TAG#URR8PPP
up vote
6
down vote
favorite
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.
c++ templates stdvector
asked 30 mins ago
Francis Cugler
3,95811227
 |Â
show 4 more comments
up vote
6
down vote
favorite
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.
c++ templates stdvector
asked 30 mins ago
Francis Cugler
3,95811227
3
It should be returning 5.12, not 6.4.
– jwimberley
24 mins ago
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
19 mins ago
2
Did you try the change that @Angew suggested.
– drescherjm
19 mins ago
2
Use less arbitrary test cases – start with ones where it's easy to tell that they're wrong.1.5, 1.5is enough to expose the bug, and if you didn't have the unnecessary special case for a one-element input,1.5would suffice.
– molbdnilo
10 mins ago
1
@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
3 mins ago
 |Â
show 4 more comments
up vote
6
down vote
favorite
up vote
6
down vote
favorite
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.
c++ templates stdvector
asked 30 mins ago
Francis Cugler
3,95811227
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.
c++ templates stdvector
c++ templates stdvector
asked 30 mins ago
Francis Cugler
3,95811227
asked 30 mins ago
Francis Cugler
3,95811227
edited 19 mins ago
asked 30 mins ago
Francis Cugler
3,95811227
asked 30 mins ago
Francis Cugler
3,95811227
asked 30 mins ago
Francis Cugler
3,95811227
3,95811227
3
It should be returning 5.12, not 6.4.
– jwimberley
24 mins ago
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
19 mins ago
2
Did you try the change that @Angew suggested.
– drescherjm
19 mins ago
2
Use less arbitrary test cases – start with ones where it's easy to tell that they're wrong.1.5, 1.5is enough to expose the bug, and if you didn't have the unnecessary special case for a one-element input,1.5would suffice.
– molbdnilo
10 mins ago
1
@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
3 mins ago
 |Â
show 4 more comments
3
It should be returning 5.12, not 6.4.
– jwimberley
24 mins ago
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
19 mins ago
2
Did you try the change that @Angew suggested.
– drescherjm
19 mins ago
2
Use less arbitrary test cases – start with ones where it's easy to tell that they're wrong.1.5, 1.5is enough to expose the bug, and if you didn't have the unnecessary special case for a one-element input,1.5would suffice.
– molbdnilo
10 mins ago
1
@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
3 mins ago
3
3
It should be returning 5.12, not 6.4.
– jwimberley
24 mins ago
It should be returning 5.12, not 6.4.
– jwimberley
24 mins ago
1
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
19 mins ago
@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
19 mins ago
2
2
Did you try the change that @Angew suggested.
– drescherjm
19 mins ago
Did you try the change that @Angew suggested.
– drescherjm
19 mins ago
2
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
10 mins ago
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
10 mins ago
1
1
@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
3 mins ago
@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
3 mins ago
 |Â
show 4 more comments
1 Answer
1
active
oldest
votes
up vote
11
down vote
accepted
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 modifyvalues. - If you call the function with a
Twhich is not valid forstd::accumulate(e.g. not arithmetic), you will get a compile-time error. The topmostifwould have to beif constexprto work the way you want it to.
answered 26 mins ago
Angew
130k11244336
Sounds good, now with theif constexpron the failed case: should I be throwingstd::runtime_error(...)like I am or should I use:static_assert(dependent_false<T>::value, "Must be arithmetic");instead? What's the major differences between the two...
– Francis Cugler
9 mins ago
@FrancisCugler It depends on what you want to achieve. If you want a compile-time error, do thestatic_assert(and you can get rid of thatifaltogether). If you actually want a runtime error, throw one.
– Angew
8 mins ago
1
@FrancisCuglerstatic_assertis evaluated at compile time, whilethrowis evaluated at runtime. Presumably you would want to check as many things as possible, at compile time.
– Algirdas Preidžius
7 mins ago
Another comment unrelated to the core of the question: Better than using thestd::is_arithmetictrait, you should write a custom trait (or a concept) to check thanTprovides a+and a/. This way, a type mimicking an arithmetic type (e.g.std::complex) could be used.
– YSC
2 mins ago
add a comment |Â
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
11
down vote
accepted
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 modifyvalues. - If you call the function with a
Twhich is not valid forstd::accumulate(e.g. not arithmetic), you will get a compile-time error. The topmostifwould have to beif constexprto work the way you want it to.
answered 26 mins ago
Angew
130k11244336
Sounds good, now with theif constexpron the failed case: should I be throwingstd::runtime_error(...)like I am or should I use:static_assert(dependent_false<T>::value, "Must be arithmetic");instead? What's the major differences between the two...
– Francis Cugler
9 mins ago
@FrancisCugler It depends on what you want to achieve. If you want a compile-time error, do thestatic_assert(and you can get rid of thatifaltogether). If you actually want a runtime error, throw one.
– Angew
8 mins ago
1
@FrancisCuglerstatic_assertis evaluated at compile time, whilethrowis evaluated at runtime. Presumably you would want to check as many things as possible, at compile time.
– Algirdas Preidžius
7 mins ago
Another comment unrelated to the core of the question: Better than using thestd::is_arithmetictrait, you should write a custom trait (or a concept) to check thanTprovides a+and a/. This way, a type mimicking an arithmetic type (e.g.std::complex) could be used.
– YSC
2 mins ago
add a comment |Â
up vote
11
down vote
accepted
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 modifyvalues. - If you call the function with a
Twhich is not valid forstd::accumulate(e.g. not arithmetic), you will get a compile-time error. The topmostifwould have to beif constexprto work the way you want it to.
answered 26 mins ago
Angew
130k11244336
Sounds good, now with theif constexpron the failed case: should I be throwingstd::runtime_error(...)like I am or should I use:static_assert(dependent_false<T>::value, "Must be arithmetic");instead? What's the major differences between the two...
– Francis Cugler
9 mins ago
@FrancisCugler It depends on what you want to achieve. If you want a compile-time error, do thestatic_assert(and you can get rid of thatifaltogether). If you actually want a runtime error, throw one.
– Angew
8 mins ago
1
@FrancisCuglerstatic_assertis evaluated at compile time, whilethrowis evaluated at runtime. Presumably you would want to check as many things as possible, at compile time.
– Algirdas Preidžius
7 mins ago
Another comment unrelated to the core of the question: Better than using thestd::is_arithmetictrait, you should write a custom trait (or a concept) to check thanTprovides a+and a/. This way, a type mimicking an arithmetic type (e.g.std::complex) could be used.
– YSC
2 mins ago
add a comment |Â
up vote
11
down vote
accepted
up vote
11
down vote
accepted
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 modifyvalues. - If you call the function with a
Twhich is not valid forstd::accumulate(e.g. not arithmetic), you will get a compile-time error. The topmostifwould have to beif constexprto work the way you want it to.
answered 26 mins ago
Angew
130k11244336
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 modifyvalues. - If you call the function with a
Twhich is not valid forstd::accumulate(e.g. not arithmetic), you will get a compile-time error. The topmostifwould have to beif constexprto work the way you want it to.
answered 26 mins ago
Angew
130k11244336
answered 26 mins ago
Angew
130k11244336
answered 26 mins ago
Angew
130k11244336
answered 26 mins ago
Angew
130k11244336
130k11244336
Sounds good, now with theif constexpron the failed case: should I be throwingstd::runtime_error(...)like I am or should I use:static_assert(dependent_false<T>::value, "Must be arithmetic");instead? What's the major differences between the two...
– Francis Cugler
9 mins ago
@FrancisCugler It depends on what you want to achieve. If you want a compile-time error, do thestatic_assert(and you can get rid of thatifaltogether). If you actually want a runtime error, throw one.
– Angew
8 mins ago
1
@FrancisCuglerstatic_assertis evaluated at compile time, whilethrowis evaluated at runtime. Presumably you would want to check as many things as possible, at compile time.
– Algirdas Preidžius
7 mins ago
Another comment unrelated to the core of the question: Better than using thestd::is_arithmetictrait, you should write a custom trait (or a concept) to check thanTprovides a+and a/. This way, a type mimicking an arithmetic type (e.g.std::complex) could be used.
– YSC
2 mins ago
add a comment |Â
Sounds good, now with theif constexpron the failed case: should I be throwingstd::runtime_error(...)like I am or should I use:static_assert(dependent_false<T>::value, "Must be arithmetic");instead? What's the major differences between the two...
– Francis Cugler
9 mins ago
@FrancisCugler It depends on what you want to achieve. If you want a compile-time error, do thestatic_assert(and you can get rid of thatifaltogether). If you actually want a runtime error, throw one.
– Angew
8 mins ago
1
@FrancisCuglerstatic_assertis evaluated at compile time, whilethrowis evaluated at runtime. Presumably you would want to check as many things as possible, at compile time.
– Algirdas Preidžius
7 mins ago
Another comment unrelated to the core of the question: Better than using thestd::is_arithmetictrait, you should write a custom trait (or a concept) to check thanTprovides a+and a/. This way, a type mimicking an arithmetic type (e.g.std::complex) could be used.
– YSC
2 mins ago
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<T>::value, "Must be arithmetic"); instead? What's the major differences between the two...– Francis Cugler
9 mins ago
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<T>::value, "Must be arithmetic"); instead? What's the major differences between the two...– Francis Cugler
9 mins ago
@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
8 mins ago
@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
8 mins ago
1
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
7 mins ago
@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
7 mins ago
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
2 mins ago
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
2 mins ago
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fstackoverflow.com%2fquestions%2f53189928%2fwhy-is-this-function-producing-incorrect-values%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
3
It should be returning 5.12, not 6.4.
– jwimberley
24 mins ago
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
19 mins ago
2
Did you try the change that @Angew suggested.
– drescherjm
19 mins ago
2
Use less arbitrary test cases – start with ones where it's easy to tell that they're wrong.
1.5, 1.5is enough to expose the bug, and if you didn't have the unnecessary special case for a one-element input,1.5would suffice.– molbdnilo
10 mins ago
1
@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
3 mins ago