Improves the GDAL clip by mask layer tool #9445

gioman · 2019-03-09T12:19:28Z

Description

Adds the option to allow choose the output resolution and the multithreaded warping option## Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

Commit messages are descriptive and explain the rationale for changes
Commits which fix bugs include fixes #11111 in the commit message next to the description
Commits which add new features are tagged with [FEATURE] in the commit message
Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and contain sufficient information in the commit message to be documented
I have read the QGIS Coding Standards and this PR complies with them
This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
New unit tests have been added for core changes
I have run the scripts/prepare-commit.sh script before each commit
Did not drink while coding

…hoose the output resolution and the multithreaded warping option

gioman · 2019-03-09T12:19:46Z

Patch by @PedroVenancio

nyalldawson

Looking good! Don't forget to update/extend the unit tests at https://github.com/qgis/QGIS/blob/master/python/plugins/processing/tests/GdalAlgorithmsTest.py#L582

nyalldawson · 2019-03-10T23:00:40Z

python/plugins/processing/algs/gdal/ClipRasterByMask.py

@@ -81,9 +94,26 @@ def initAlgorithm(self, config=None):
                                                        self.tr('Match the extent of the clipped raster to the extent of the mask layer'),
                                                        defaultValue=True))
        self.addParameter(QgsProcessingParameterBoolean(self.KEEP_RESOLUTION,
-                                                        self.tr('Keep resolution of output raster'),
+                                                        self.tr('Keep resolution of input raster'),


Is this a bug fix? Would be good to separate this commit so it can be backported

nyalldawson · 2019-03-10T23:02:10Z

python/plugins/processing/algs/gdal/ClipRasterByMask.py

                                                        defaultValue=False))
-
+        self.addParameter(QgsProcessingParameterBoolean(self.SET_RESOLUTION,


To me this parameter seems redundant -- we could cover the same functionality by testing if X_RESOLUTION and Y_RESOLUTION are set and not None. (and throw an exception if only one is set and not the other)

nyalldawson · 2019-03-10T23:02:18Z

python/plugins/processing/algs/gdal/ClipRasterByMask.py

+                                                        self.tr('Set output file resolution'),
+                                                        defaultValue=False))
+        self.addParameter(QgsProcessingParameterNumber(self.X_RESOLUTION,
+                                                       self.tr('X Resolution to output bands'),


Suggested change

self.tr('X Resolution to output bands'),

self.tr('X Resolution for output bands'),

nyalldawson · 2019-03-10T23:02:28Z

python/plugins/processing/algs/gdal/ClipRasterByMask.py

+                                                       defaultValue=None,
+                                                       optional=True))
+        self.addParameter(QgsProcessingParameterNumber(self.Y_RESOLUTION,
+                                                       self.tr('Y Resolution to output bands'),


Suggested change

self.tr('Y Resolution to output bands'),

self.tr('Y Resolution for output bands'),

nyalldawson · 2019-03-10T23:02:48Z

python/plugins/processing/algs/gdal/ClipRasterByMask.py

+                                                       optional=True))
+        multithreading_param = QgsProcessingParameterBoolean(self.MULTITHREADING,
+                                                             self.tr('Use multithreaded warping implementation'),
+                                                             defaultValue=False)


Any reason this should default to true?

nyalldawson · 2019-03-10T23:04:10Z

I just noticed this is targeted at 3.4 -- is that valid? Looks like a new feature to me.

gioman · 2019-03-11T12:04:36Z

I just noticed this is targeted at 3.4 -- is that valid? Looks like a new feature to me.

@nyalldawson yes I see your point. Is not really a bug fix.

PedroVenancio · 2019-03-11T15:21:20Z

Hi @nyalldawson

This feature/pull request was a mix of:

Bug fix (description 'Keep resolution of input raster')
Prevent a bug from GDAL, using -s_srs and -t_srs options (Unwanted datum shift with GDAL >= 2.3 and PROJ 5 OSGeo/gdal#1156
http://osgeo-org.1560.x6.nabble.com/gdal-dev-gdalwarp-cutline-shift-between-raster-and-vector-in-GDAL-2-3-2-td5389268.html )
Bring to QGIS 3 a feature that was already in QGIS 2 (output file resolution option - https://github.com/qgis/QGIS/blob/release-2_18/python/plugins/GdalTools/tools/doClipper.py)

The proposal for the backport was for that reasons, but I understand that there are some changes, and can be considered new features.

@nyalldawson Please, change the code where you consider that it can be improved. Although it is functional, I believe the code can be much more polite.

I see one problem in it, as it is using QgsProcessingParameterBoolean (and not radio buttons). So, a user can check both KEEP_RESOLUTION and SET_RESOLUTION at the same time. Is there any form to put them mutually exclusive in Processing? If not, I think a user needs to know what is doing when activate an option that is, by default, unselected.

Improves the GDAL clip by mask layer tool: adds the option to allow c…

5383bf8

…hoose the output resolution and the multithreaded warping option

m-kuhn added the Backport Is a backport of another pull request label Mar 10, 2019

nyalldawson reviewed Mar 10, 2019

View reviewed changes

gioman closed this Mar 11, 2019

qgib mentioned this pull request May 25, 2019

Set output file resolution option and CRS options to Processing Clip Raster to Mask Layer - ClipRasterByMask.py #29069

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improves the GDAL clip by mask layer tool #9445

Improves the GDAL clip by mask layer tool #9445

gioman commented Mar 9, 2019

gioman commented Mar 9, 2019

nyalldawson left a comment

nyalldawson Mar 10, 2019

nyalldawson Mar 10, 2019

nyalldawson Mar 10, 2019

nyalldawson Mar 10, 2019

nyalldawson Mar 10, 2019

nyalldawson commented Mar 10, 2019

gioman commented Mar 11, 2019

PedroVenancio commented Mar 11, 2019

		defaultValue=False))

		self.addParameter(QgsProcessingParameterBoolean(self.SET_RESOLUTION,

	self.tr('X Resolution to output bands'),
	self.tr('X Resolution for output bands'),

	self.tr('Y Resolution to output bands'),
	self.tr('Y Resolution for output bands'),

Improves the GDAL clip by mask layer tool #9445

Improves the GDAL clip by mask layer tool #9445

Conversation

gioman commented Mar 9, 2019

Description

gioman commented Mar 9, 2019

nyalldawson left a comment

Choose a reason for hiding this comment

nyalldawson Mar 10, 2019

Choose a reason for hiding this comment

nyalldawson Mar 10, 2019

Choose a reason for hiding this comment

nyalldawson Mar 10, 2019

Choose a reason for hiding this comment

nyalldawson Mar 10, 2019

Choose a reason for hiding this comment

nyalldawson Mar 10, 2019

Choose a reason for hiding this comment

nyalldawson commented Mar 10, 2019

gioman commented Mar 11, 2019

PedroVenancio commented Mar 11, 2019