[processing] port heatmap algorithm #4843

alexbruy · 2017-07-12T07:25:46Z

Description

Restore Heatmap algorithm (no tests yet)

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 containt 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

alexbruy · 2017-07-12T07:27:44Z

BTW, I noticed that after executing any already available QGIS algorithm there is an error like this

Python error[1] Traceback (most recent call last):
  File "/tmp/build/qgis/output/python/plugins/processing/algs/qgis/QgisAlgorithm.py", line 39, in shortHelpString
    return shortHelp.get(self.id(), None)
RuntimeError: wrapped C/C++ object of type Heatmap has been deleted

nyalldawson

Thanks!

I'm really keen for feedback on how you found the porting process & the new API. Any suggestions for improvement?

nyalldawson · 2017-07-12T08:35:07Z

python/plugins/processing/algs/qgis/Heatmap.py

-    def displayName(self):
-        return self.tr('Heatmap (Kernel Density Estimation)')
+        self.addParameter(QgsProcessingParameterRasterDestination(self.OUTPUT_LAYER, self.tr('Heatmap')))
+        self.addOutput(QgsProcessingOutputRasterLayer(self.OUTPUT_LAYER, self.tr('Heatmap')))


This is no longer required - since 058271b the output will automatically be created

nyalldawson · 2017-07-12T08:37:07Z

python/plugins/processing/algs/qgis/Heatmap.py

-        radius = self.getParameterValue(self.RADIUS)
-        kernel_shape = self.getParameterValue(self.KERNEL)
+        radius = self.parameterAsDouble(parameters, self.RADIUS, context)
+        kernel_shape = self.parameterAsEnums(parameters, self.KERNEL, context)


Should be parameterAsEnum - parameterAsEnums will return a list and is for multiple value parameters only

nyalldawson · 2017-07-12T08:37:13Z

python/plugins/processing/algs/qgis/Heatmap.py

-        output_values = self.getParameterValue(self.OUTPUT_VALUE)
-        output = self.getOutputValue(self.OUTPUT_LAYER)
+        decay = self.parameterAsDouble(parameters, self.DECAY, context)
+        output_values = self.parameterAsEnums(parameters, self.OUTPUT_VALUE, context)


nyalldawson · 2017-07-12T08:38:07Z

python/plugins/processing/algs/qgis/Heatmap.py

@@ -170,7 +204,7 @@ def processAlgorithm(self, parameters, context, feedback):
        kde = QgsKernelDensityEstimation(kde_params, output, output_format)

        if kde.prepare() != QgsKernelDensityEstimation.Success:
-            raise GeoAlgorithmExecutionException(
+            raise QgsProcessingException(


it should be fine to keep throwing GeoAlgorithmExecutionException - there's code which handles this and maps it automatically to the c++ exception. I guess there's no harm either way

So it is better to use GeoAlgorithmExecutionException or switch to QgsProcessingException to reduce (a little) code to maintain?

Yeah, you're right. Let's reduce the workload and switch everything over.

nyalldawson · 2017-07-12T08:39:00Z

python/plugins/processing/algs/qgis/QGISAlgorithmProvider.py

@@ -78,6 +78,7 @@
 from .VectorSplit import VectorSplit
 from .VoronoiPolygons import VoronoiPolygons
 from .ZonalStatistics import ZonalStatistics
+from .Heatmap import Heatmap


Mind keeping the alphabetical sorting that's been started here? (and below)

(sorry, pedantic I know!)

nyalldawson · 2017-07-12T08:40:41Z

I noticed that after executing any already available QGIS algorithm there is an error like this

It's on my list - there's some weird issue going on buried within the hidden depths of sip. I'll find it....

nyalldawson · 2017-07-12T08:48:44Z

python/plugins/processing/algs/qgis/Heatmap.py

@@ -179,10 +213,12 @@ def processAlgorithm(self, parameters, context, feedback):
        total = 100.0 / layer.featureCount() if layer.featureCount() else 0
        for current, f in enumerate(features):
            if kde.addFeature(f) != QgsKernelDensityEstimation.Success:


Also - better add a check for feedback.isCanceled() here too and break out of the loop

alexbruy · 2017-07-12T17:23:30Z

Also added test (failing for now). Trying to debug what is wrong, probably I did something wrong when porting.

nyalldawson · 2017-07-12T21:08:30Z

Does the test pass locally? For me none of the raster hash test values match locally vs Travis (likely the library version differences). I just (make sure the algorithm works) then set them to Travis, and suffer the local failures. Maybe we should make the hash values a list of acceptable values instead?

alexbruy · 2017-07-13T07:49:43Z

No test does not pass locally. Seems there is something wrong with algorithm (my port) itself

alexbruy · 2017-07-13T09:13:22Z

@nyalldawson hopefully test and algorithm now fixed. I will add comments to the documentation PR

nyalldawson · 2017-07-13T09:18:06Z

What was the issue?

alexbruy · 2017-07-13T09:32:38Z

Initial output raster declaration

self.addOutput(QgsProcessingOutputRasterLayer(self.OUTPUT_LAYER, self.tr('Heatmap')))

was wrong and even removing it and using automatically generated output does not helped (probably I just don't know how to use them correctly). It worked after declaring output raster explicitly similarly to the Aspect algorithm

self.addParameter(QgsProcessingParameterRasterDestination(self.OUTPUT_LAYER, self.tr('Heatmap')))

nyalldawson · 2017-07-13T10:06:37Z

Ok - I definitely need to add a lot of detail about that to the porting guide :)

alexbruy requested a review from nyalldawson July 12, 2017 07:25

nyalldawson reviewed Jul 12, 2017

View reviewed changes

alexbruy added 3 commits July 13, 2017 12:11

[processing] port heatmap algorithm

229d2f1

[processing] add test for heatmap algorithm

b01ffa5

more test fixes

8285712

nyalldawson merged commit cfbed91 into qgis:master Jul 13, 2017

alexbruy deleted the processing-heatmap branch July 13, 2017 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[processing] port heatmap algorithm #4843

[processing] port heatmap algorithm #4843

alexbruy commented Jul 12, 2017 •

edited

Loading

alexbruy commented Jul 12, 2017

nyalldawson left a comment

nyalldawson Jul 12, 2017

nyalldawson Jul 12, 2017

nyalldawson Jul 12, 2017

nyalldawson Jul 12, 2017

alexbruy Jul 12, 2017

nyalldawson Jul 12, 2017

nyalldawson Jul 12, 2017

nyalldawson commented Jul 12, 2017

nyalldawson Jul 12, 2017

alexbruy commented Jul 12, 2017

nyalldawson commented Jul 12, 2017

alexbruy commented Jul 13, 2017

alexbruy commented Jul 13, 2017

nyalldawson commented Jul 13, 2017

alexbruy commented Jul 13, 2017

nyalldawson commented Jul 13, 2017

[processing] port heatmap algorithm #4843

[processing] port heatmap algorithm #4843

Conversation

alexbruy commented Jul 12, 2017 • edited Loading

Description

Checklist

alexbruy commented Jul 12, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyalldawson commented Jul 12, 2017

Choose a reason for hiding this comment

alexbruy commented Jul 12, 2017

nyalldawson commented Jul 12, 2017

alexbruy commented Jul 13, 2017

alexbruy commented Jul 13, 2017

nyalldawson commented Jul 13, 2017

alexbruy commented Jul 13, 2017

nyalldawson commented Jul 13, 2017

alexbruy commented Jul 12, 2017 •

edited

Loading