Skip to content
Permalink
Browse files
Check validity of input geometries in intersection algorithm, take 2 
Fail if invalid geometries are found.
And some easy performance wins. Just because.

Fix #11986
  • Loading branch information
@m-kuhn
m-kuhn committed Oct 20, 2016
1 parent 01e570c commit 5ae0e784e78993870c416ff499616a5147803c2c
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 5 deletions.
32 python/plugins/processing/algs/qgis/Intersection.py
View file
@@ -34,7 +34,7 @@
from processing.core.GeoAlgorithm import GeoAlgorithm
from processing.core.ProcessingLog import ProcessingLog
from processing.core.GeoAlgorithmExecutionException import GeoAlgorithmExecutionException
from processing.core.parameters import ParameterVector
from processing.core.parameters import ParameterVector, ParameterBoolean
from processing.core.outputs import OutputVector
from processing.tools import dataobjects, vector

@@ -54,6 +54,7 @@ class Intersection(GeoAlgorithm):

INPUT = 'INPUT'
INPUT2 = 'INPUT2'
IGNORE_NULL = 'IGNORE_NULL'
OUTPUT = 'OUTPUT'

def getIcon(self):
@@ -66,13 +67,17 @@ def defineCharacteristics(self):
self.tr('Input layer'), [ParameterVector.VECTOR_TYPE_ANY]))
self.addParameter(ParameterVector(self.INPUT2,
self.tr('Intersect layer'), [ParameterVector.VECTOR_TYPE_ANY]))
self.addParameter(ParameterBoolean(Intersection.IGNORE_NULL,
self.tr('Ignore NULL geometries'),
False, True))
self.addOutput(OutputVector(self.OUTPUT, self.tr('Intersection')))

def processAlgorithm(self, progress):
vlayerA = dataobjects.getObjectFromUri(
self.getParameterValue(self.INPUT))
vlayerB = dataobjects.getObjectFromUri(
self.getParameterValue(self.INPUT2))
ignoreNull = self.getParameterValue(Intersection.IGNORE_NULL)

geomType = QgsWKBTypes.multiType(QGis.fromOldWkbType(vlayerA.wkbType()))
fields = vector.combineVectorFields(vlayerA, vlayerB)
@@ -84,13 +89,30 @@ def processAlgorithm(self, progress):
total = 100.0 / len(selectionA)
for current, inFeatA in enumerate(selectionA):
progress.setPercentage(int(current * total))
geom = QgsGeometry(inFeatA.geometry())
geom = inFeatA.geometry()
if not geom:
if ignoreNull:
continue
else:
raise GeoAlgorithmExecutionException(
self.tr('Input layer A contains NULL geometries. Please check "Ignore NULL geometries" if you want to run this algorithm anyway.'))
if not geom.isGeosValid():
raise GeoAlgorithmExecutionException(
self.tr('Input layer A contains invalid geometries (Feature {}). Unable to complete intersection algorithm.'.format(inFeatA.id())))
atMapA = inFeatA.attributes()
intersects = index.intersects(geom.boundingBox())
for i in intersects:
request = QgsFeatureRequest().setFilterFid(i)
inFeatB = vlayerB.getFeatures(request).next()
for inFeatB in vlayerB.getFeatures(QgsFeatureRequest().setFilterFids(intersects)):
tmpGeom = QgsGeometry(inFeatB.geometry())
if not geom:

This comment has been minimized.

Sign in to view

Copy link
@nyalldawson

nyalldawson Oct 20, 2016

Collaborator

How would this be possible? If it has no geometry it wouldn't be in the intersects list in the first place...

This comment has been minimized.

Sign in to view

Copy link
@m-kuhn

m-kuhn Oct 20, 2016

Author Member

Good point
if ignoreNull:
continue
else:
raise GeoAlgorithmExecutionException(
self.tr('Input layer B contains NULL geometries. Please check "Ignore NULL geometries" if you want to run this algorithm anyway.'))
if not geom.isGeosValid():
raise GeoAlgorithmExecutionException(
self.tr('Input layer B contains invalid geometries (Feature {}). Unable to complete intersection algorithm.'.format(inFeatB.id())))

if geom.intersects(tmpGeom):
atMapB = inFeatB.attributes()
int_geom = QgsGeometry(geom.intersection(tmpGeom))
1 python/plugins/processing/tests/testdata/qgis_algorithm_tests.yaml
View file
@@ -124,6 +124,7 @@ tests:
INPUT2:
name: polys.gml
type: vector
IGNORE_NULL: True
results:
OUTPUT:
name: expected/intersection_collection_fallback.shp

8 comments on commit 5ae0e78

@m-kuhn
Copy link
Member Author

@m-kuhn m-kuhn commented on 5ae0e78 Oct 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@volaya @nyalldawson @gioman

Touching base for an earlier discussion w.r.t "fail-fast" algorithms. This commit outlines the way I think it should be handled.

  1. Check for NULL geometries
    • By default error out
    • Unless the user checked "Ignore NULL geometries"
  2. Check for invalid geometries
    • Error out. I'd like to tweak the wording to give the user hints what he could do to fix the geometries. Any hints?

If you are ok with this, it could be put into some utils library to avoid copying / pasting it a million times around.

@nyalldawson
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach means it's impossible to keep null geometries. I think ignoring should instead copy the feature without geometry to the output layer. Then you'd either be explicitly strip them out in adjacent (with a new algorithm for this purpose) or copying them unchanged.

I thought you were thinking of making this a global processing option too?

@m-kuhn
Copy link
Member Author

@m-kuhn m-kuhn commented on 5ae0e78 Oct 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right, NULL geometries will disappear. My approach was to extend SQL logic where NULL != NULL. We could as well copy any NULL geometries from both layers to the result. Or the cartesian product of combining every NULL entry with every entry from the other layer.

For this commit, the main goal was to fixup invalid geometry problems and not yet the ultimate solution to all. That will hopefully be in QGIS 3 after this discussion here ;)

@nyalldawson
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine... I actually didn't even think through the full implications of this for intersects. So I agree - intersects should skip null geometries, and it never makes sense to copy them.

My thoughts:

  • routines which operate on a geometry - eg buffer, etc should be able to copy null geometries intact
  • routines which do some type of spatial filtering eg selection by location, this intersection alg, should skip over null geometry features

So ignore my original comment, this seems like the right approach until we move that setting to a global property.

Should we quickly make a "strip null geometry" alg before release?

@nirvn
Copy link
Contributor

@nirvn nirvn commented on 5ae0e78 Oct 21, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m-kuhn , shouldn't the same be applied to other vector overlay operations?

@m-kuhn
Copy link
Member Author

@m-kuhn m-kuhn commented on 5ae0e78 Oct 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thoughts:

  • routines which operate on a geometry - eg buffer, etc should be able to copy null geometries intact
  • routines which do some type of spatial filtering eg selection by location, this intersection alg, should skip over null geometry features

that sounds like a good approach 👍 , although not sure about the select by location, I think that one could offer a checkbox to decide what to do with nulls

@m-kuhn
Copy link
Member Author

@m-kuhn m-kuhn commented on 5ae0e78 Oct 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m-kuhn , shouldn't the same be applied to other vector overlay operations?

yes!

@m-kuhn
Copy link
Member Author

@m-kuhn m-kuhn commented on 5ae0e78 Oct 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

routines which do some type of spatial filtering eg selection by location, this intersection alg, should skip over null geometry features

Yet another situation: I think for the Union algorithm it makes sense to copy the features to the target layer.

Please sign in to comment.