Bug report #16925

Field calculator "round" function wrong results if rounding to 8 or more decimals

Added by Giovanni Manghi almost 3 years ago. Updated over 1 year ago.

Status:Closed
Priority:Normal
Assignee:-
Category:Field calculator
Affected QGIS version:2.18.11 Regression?:No
Operating System: Easy fix?:No
Pull Request or Patch supplied:No Resolution:fixed/implemented
Crashes QGIS or corrupts data:No Copied to github as #:24824

Description

This is regardless of the datasource and field type, example:

round(40.70298315437546, 8) > -21.47483648

round(40.70298315437546, 7) > 40.7029832

Associated revisions

Revision 4b009f96
Added by Nyall Dawson almost 3 years ago

Use std::round instead of qRound

Now that our minimum VS studio version allowed supports std::round,
we should use that in place of Qt's qRound method.

Because:
- it doesn't truncate to int, resulting in unpredictable
behaviour (refs #16925)
- better to stick to standard c++ methods wherever possible,
since they're likely better supported and optimised by the
compilers
- it's a tiny reduction to the barrier for entry to QGIS
development (I'm sick of pointing out the need to use
qRound during PR reviews!)

History

#1 Updated by Alain Bertholom almost 3 years ago

In core/qgsexpression.cpp fcnRound:
qRound(...) is called for rounding, an int is returned.
use the math round function instead of qRound should solve the problem.

The field calculator should return a floating point number in all case...

static QVariant fcnRound( const QVariantList& values, const QgsExpressionContext , QgsExpression parent ) {
if ( values.length() == 2 ) {
double number = getDoubleValue( values.at( 0 ), parent );
double scaler = pow( 10.0, getIntValue( values.at( 1 ), parent ) );
//return QVariant( qRound( number * scaler ) / scaler );
return QVariant( round( number * scaler ) / scaler );
}

if ( values.length() == 1 )
{
double number = getIntValue( values.at( 0 ), parent );
//return QVariant( qRound( number ) ).toInt();
return QVariant( round( number ) );
}
return QVariant();
}

#2 Updated by Nyall Dawson almost 3 years ago

Nice fix Alain - can you open a pull request on GitHub with this change? That'll make it easy to review.

#3 Updated by Alain Bertholom almost 3 years ago

On master, 2.18, or both?

#4 Updated by Jürgen Fischer almost 3 years ago

Alain Bertholom wrote:

On master, 2.18, or both?

Note that 2.18 is built with VS2010 on Windows, which doesn't have std::round. See 8cb578f69, 0a1270b9f, ad437bfdff and 4f58f13822

#5 Updated by Nyall Dawson almost 3 years ago

You can use qgsRound for 2.18, instead of qRound.

#6 Updated by Giovanni Manghi over 1 year ago

  • Resolution set to fixed/implemented
  • Status changed from Open to Closed

Works as expected on master.

Also available in: Atom PDF