[feature] add native Round raster algorithm #35694

root676 · 2020-04-09T19:19:45Z

Description

This PR proposes a new processing algorithm to round Raster values (thanks @nyalldawson, your algorithm list triggered me ;) ). The algorithm is a reimplementation of the FME counterpart RasterCellValueRounder.

The main functionality of the algorithm is either standard up/nearest/down-rounding of floating point cell values or rounding to powers of a user specified base n. By exposing the base as parameter to the user, the algorithm now is even a bit more versatile than the FME counterpart and allows for some interesting raster generalization (see example). The algorithm can be used with rasters of all types (even byte/integer rasters when rounding to powers of n) and outputs the same raster data type, a standard floating point rounding of an integer raster will copy it and raise a warning. A detailed description of the parameters interplay can be found in the algorithm helpstring.
The PR also contains a test of the algorithm.

Example:

Input Raster file

Output raster rounded to multiples of 10

Please feel free to review/comment!

roya0045 · 2020-04-09T19:24:57Z

Isn't that smooth?

And also looking at the code, this made me think.
Isn't there a way to convert the images to an array and then reproject them? Having a complete raster as an array would be far better to control like in numpy. Though this might not be easy to do with the type of data we are dealing with.

nyalldawson

Nice addition! You've nailed the implementation too (like usual 😉 )

nyalldawson · 2020-04-10T01:34:03Z

images/images.qrc

@@ -116,6 +116,7 @@
        <file>themes/default/algorithms/mAlgorithmRandomPointsWithinPolygon.svg</file>
        <file>themes/default/algorithms/mAlgorithmRandomPointsWithinExtent.svg</file>
        <file>themes/default/algorithms/mAlgorithmRegularPoints.svg</file>
+	<file>themes/default/algorithms/mAlgorithmRoundRastervalues.svg</file>


indentation

nyalldawson · 2020-04-10T01:35:44Z

src/analysis/processing/qgsalgorithmroundrastervalues.cpp

+
+QString QgsRoundRasterValuesAlgorithm::group() const
+{
+  return QObject::tr( "Raster tools" );


I'm not sure Raster Tools is the right group for this. I think it fits better in the current Raster Analysis group, but ideally we'd split that group into purely analysis tools vs "manipulation" tools.

I agree. One question on adding new processing algorithm groups between major releases: Is this considered as an API break? I plan to add more raster creation algorithms and would put them in one new group. Is it problematic to regroup algorithms/add new groups between major releases?

It's not an API break, I think it's perfectly acceptable for a minor release!

nyalldawson · 2020-04-10T01:37:24Z

src/analysis/processing/qgsalgorithmroundrastervalues.cpp

+  addParameter( new QgsProcessingParameterRasterLayer( QStringLiteral( "INPUT" ), QStringLiteral( "Input raster" ) ) );
+  addParameter( new QgsProcessingParameterBand( QStringLiteral( "BAND" ), QObject::tr( "Band number" ), 1, QStringLiteral( "INPUT" ) ) );
+  addParameter( new QgsProcessingParameterEnum( QStringLiteral( "ROUNDING_DIRECTION" ), QObject::tr( "Rounding direction" ), QStringList() << QObject::tr( "Round up" ) << QObject::tr( "Round to nearest" ) << QObject::tr( "Round down" ), false, 1 ) );
+  addParameter( new QgsProcessingParameterNumber( QStringLiteral( "DECIMAL_PLACES" ), QObject::tr( "Number of decimals places (use negative values to round cell values to a multiple of a base n, see advanced parameters)" ), QgsProcessingParameterNumber::Integer, 2 ) );


This description is very long compared with the usual parameter description. I'd shorten to just "number of decimal places" and leave the full explanation for the help string.

I keep thinking we need api to support extended descriptions for parameters, which we could show in a tooltip...

Agreed - this would be very nice!

nyalldawson · 2020-04-10T01:37:45Z

src/analysis/processing/qgsalgorithmroundrastervalues.cpp

+
+QString QgsRoundRasterValuesAlgorithm::shortHelpString() const
+{
+  return QObject::tr( "This algorithm rounds the cell values of a rasterdataset according to the specified number of decimals.\n "


Suggested change

return QObject::tr( "This algorithm rounds the cell values of a rasterdataset according to the specified number of decimals.\n "

return QObject::tr( "This algorithm rounds the cell values of a raster dataset according to the specified number of decimals.\n "

nyalldawson · 2020-04-10T01:38:07Z

src/analysis/processing/qgsalgorithmroundrastervalues.cpp

+                      "Alternatively, a negative number of decimal places may be used to round values to powers of a base n "
+                      "(specified in the advanced parameter Base n). For example, with a Base value n of 10 and Decimal places of -1 "
+                      "the algorithm rounds cell values to multiples of 10, -2 rounds to multiples of 100, and so on. Arbitrary base values "
+                      "may be chosen, the algorithm applies the same multiplicative principle.Rounding cell values to multiples of "


Suggested change

"may be chosen, the algorithm applies the same multiplicative principle.Rounding cell values to multiples of "

"may be chosen, the algorithm applies the same multiplicative principle. Rounding cell values to multiples of "

nyalldawson · 2020-04-10T01:39:03Z

src/analysis/processing/qgsalgorithmroundrastervalues.h

+    double roundUpBaseN( double &value, int &multipleOfBaseN );
+    double roundDownBaseN( double &value, int &multipleOfBaseN );
+
+    QgsRasterLayer *mInputRaster;


Better not to store this as a member -- it potentially risks someone using it from processAlgorithm later.

Do you mean the QgsRasterLayer member variable? If so, it is fixed.

nyalldawson · 2020-04-10T01:41:37Z

src/analysis/processing/qgsalgorithmroundrastervalues.cpp

+          break;
+        for ( int column = 0; column < iterCols; column++ )
+        {
+          if ( analysisRasterBlock->isNoData( row, column ) )


Calling valueAndNoData once here instead of separately calling isNoData and the value is more efficient.

nyalldawson · 2020-04-10T01:43:03Z

src/analysis/processing/qgsalgorithmroundrastervalues.cpp

+double QgsRoundRasterValuesAlgorithm::roundNearest( double &value, int &decimals )
+{
+  double m = ( value < 0.0 ) ? -1.0 : 1.0;
+  double scaleFactor = std::pow( 10.0, decimals );


This scalefactor is getting calculated once per pixel -- it'd be preferable to calculate it once in processAlgorithm and pass it to this function.

Sorry, I think you misunderstood me -- it's "scale factor" which is a constant value and should only be calculated once

nyalldawson · 2020-04-10T01:45:12Z

@roya0045

Isn't there a way to convert the images to an array

That's exactly what QgsRasterBlock is

root676 · 2020-04-10T10:49:01Z

@nyalldawson thank you for the fast review :)

roya0045 · 2020-04-10T11:26:10Z

@nyalldawson I just find it strange that values must be handled and set one at the time instead of using broadcasting. Though it still might me more efficient in c++ than numpy.

nyalldawson

Nearly there!

nyalldawson · 2020-04-10T19:43:46Z

src/analysis/processing/qgsalgorithmroundrastervalues.cpp

+  addParameter( new QgsProcessingParameterEnum( QStringLiteral( "ROUNDING_DIRECTION" ), QObject::tr( "Rounding direction" ), QStringList() << QObject::tr( "Round up" ) << QObject::tr( "Round to nearest" ) << QObject::tr( "Round down" ), false, 1 ) );
+  addParameter( new QgsProcessingParameterNumber( QStringLiteral( "DECIMAL_PLACES" ), QObject::tr( "Number of decimals places" ), QgsProcessingParameterNumber::Integer, 2 ) );
+  addParameter( new QgsProcessingParameterRasterDestination( QStringLiteral( "OUTPUT" ), QObject::tr( "Output raster" ) ) );
+  std::unique_ptr< QgsProcessingParameterDefinition > baseParameter = qgis::make_unique< QgsProcessingParameterNumber >( QStringLiteral( "BASE_N" ), QObject::tr( "Base n (provides the base rounding raster values near/up/down to multiples of n when Decimal parameter is negative)" ), QgsProcessingParameterNumber::Integer, 10, true, 1 );


Better shorten this one too

I condensed the parameter description a little.

nyalldawson · 2020-04-10T19:44:47Z

src/analysis/processing/qgsalgorithmroundrastervalues.cpp

+double QgsRoundRasterValuesAlgorithm::roundNearest( double &value, int &decimals )
+{
+  double m = ( value < 0.0 ) ? -1.0 : 1.0;
+  double scaleFactor = std::pow( 10.0, decimals );


Sorry, I think you misunderstood me -- it's "scale factor" which is a constant value and should only be calculated once

nyalldawson · 2020-04-10T19:45:59Z

src/analysis/processing/qgsalgorithmroundrastervalues.cpp

+  return ( std::round( value * m * scaleFactor ) / scaleFactor ) * m;
+}
+
+double QgsRoundRasterValuesAlgorithm::roundUp( double &value, double &m, int &decimals )


These arguments should be passed by value, not reference (by reference is slower then by value for small trivially copied types like int and double)

nyalldawson · 2020-04-10T23:07:45Z

Perfect!

[feature] add native Round raster algorithm

ad16f5d

github-actions bot added this to the 3.14.0 milestone Apr 9, 2020

root676 added 2 commits April 9, 2020 21:52

spelling fixes

b673cda

another spelling fix

ab8d5b9

nyalldawson reviewed Apr 10, 2020

View reviewed changes

nyalldawson added Feature Processing Relating to QGIS Processing framework or individual Processing algorithms labels Apr 10, 2020

root676 added 6 commits April 10, 2020 11:28

fix intendation

f31ed3f

update icon

2d01e26

change algorithm group

97b6263

remove input raster layer as member

2ff00d2

use valueAndNoData() for retrieving values

4f49ab1

move scale factor to processAlgorithm()

da36f94

nyalldawson reviewed Apr 10, 2020

View reviewed changes

address parameter length and rounding function optimization

af9ff93

nyalldawson approved these changes Apr 10, 2020

View reviewed changes

nyalldawson merged commit bea2834 into qgis:master Apr 12, 2020

root676 deleted the processing_roundRaster branch April 13, 2020 08:39

timlinux added Changelog Items that are queued to appear in the visual changelog - remove after harvesting and removed Changelog Items that are queued to appear in the visual changelog - remove after harvesting labels May 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] add native Round raster algorithm #35694

[feature] add native Round raster algorithm #35694

root676 commented Apr 9, 2020

roya0045 commented Apr 9, 2020

nyalldawson left a comment

nyalldawson Apr 10, 2020

root676 Apr 10, 2020

nyalldawson Apr 10, 2020

root676 Apr 10, 2020 •

edited

nyalldawson Apr 10, 2020

nyalldawson Apr 10, 2020

root676 Apr 10, 2020

root676 Apr 10, 2020

nyalldawson Apr 10, 2020

root676 Apr 10, 2020

nyalldawson Apr 10, 2020

root676 Apr 10, 2020

nyalldawson Apr 10, 2020

root676 Apr 10, 2020

nyalldawson Apr 10, 2020

root676 Apr 10, 2020

nyalldawson Apr 10, 2020

root676 Apr 10, 2020

nyalldawson Apr 10, 2020

nyalldawson commented Apr 10, 2020

root676 commented Apr 10, 2020

roya0045 commented Apr 10, 2020

nyalldawson left a comment

nyalldawson Apr 10, 2020

root676 Apr 10, 2020

nyalldawson Apr 10, 2020

nyalldawson Apr 10, 2020

root676 Apr 10, 2020

nyalldawson commented Apr 10, 2020

	return QObject::tr( "This algorithm rounds the cell values of a rasterdataset according to the specified number of decimals.\n "
	return QObject::tr( "This algorithm rounds the cell values of a raster dataset according to the specified number of decimals.\n "

	"may be chosen, the algorithm applies the same multiplicative principle.Rounding cell values to multiples of "
	"may be chosen, the algorithm applies the same multiplicative principle. Rounding cell values to multiples of "

[feature] add native Round raster algorithm #35694

[feature] add native Round raster algorithm #35694

Conversation

root676 commented Apr 9, 2020

Description

roya0045 commented Apr 9, 2020

nyalldawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

root676 Apr 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyalldawson commented Apr 10, 2020

root676 commented Apr 10, 2020

roya0045 commented Apr 10, 2020

nyalldawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyalldawson commented Apr 10, 2020

root676 Apr 10, 2020 •

edited