Bug report #9297

ftools: warn users in clear, meaningful, visible way when geometries have errors (was : "ftools difference: wrong result")

Added by Giovanni Manghi almost 6 years ago. Updated over 2 years ago.

Status:Closed
Priority:Severe/Regression
Assignee:-
Category:Processing/QGIS
Affected QGIS version:2.18.0 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 #:17905

Description

With the attached layers the difference operation (ftools) returns wrong results.

Compare with the difference you can do in processing with GRASS or SAGA.

difference.zip (14.5 KB) Giovanni Manghi, 2014-01-06 12:59 AM


Related issues

Related to QGIS Application - Bug report #15962: Union & Intersection results are Incorrect Closed 2016-12-08

Associated revisions

Revision 143cfab1
Added by Matthias Kuhn over 3 years ago

[processing] Difference: don't ignore invalid geometries by default

Fix #9297

Revision c1e1b287
Added by Matthias Kuhn over 3 years ago

[processing] Difference: don't ignore invalid geometries by default

Fix #9297

History

#1 Updated by Giovanni Manghi over 5 years ago

  • Target version changed from Future Release - High Priority to Version 2.4

#2 Updated by Jürgen Fischer over 5 years ago

  • Target version changed from Version 2.4 to Future Release - High Priority

#3 Updated by Giovanni Manghi over 3 years ago

  • Subject changed from ftools difference: wrong result to ftools: warn users in clear, meaningful, visible way when geometries have errors (was : "ftools difference: wrong result")
  • Priority changed from Normal to Severe/Regression
  • Target version changed from Future Release - High Priority to Version 2.16

This is still true and the latest master.

The problem is in the geometry of feature with internal id 16 in "shapefile2", once fixed the operation is ok.

But the real problem is that the QGIS log GEOS message that warns about it is not really visible and meaningful for the vast majority of users.

It is acceptable that QGIS/ftools may fail if there are geometries with errors, but the users must be warned in a meaningful, clear and visible way.

#4 Updated by Alexander Bruy over 3 years ago

We can change Processing behaviour to throw error and stop algorithm if there are invalid geometries in input layers. Is it OK?

#5 Updated by Giovanni Manghi over 3 years ago

Alexander Bruy wrote:

We can change Processing behaviour to throw error and stop algorithm if there are invalid geometries in input layers. Is it OK?

from my point of view is a big +1.

#6 Updated by Nathan Woodrow over 3 years ago

Is there any alternative? Flag and report at the end? IMO it's annoying when a tool stops on something that it can just ignore and move on. Invalid geometries are a fact of life and sometimes out of the control of the person running the algorithm.

#7 Updated by Giovanni Manghi over 3 years ago

Nathan Woodrow wrote:

IMO it's annoying when a tool stops on something that it can just ignore and move on.

but it is not really that easy: in this case (but also others, I'm putting together a report on the "union" tool) ignoring the geometry error produces a wrong result! If the input layers do not contain many features maybe the error is easy to spot, but usually is not the case, and the user is left with a very bad issue without knowing (right now there is just a log message, no message bar message).

Invalid geometries are a fact of life and sometimes out of the control of the person running the algorithm.

yes, but as I wrote above this is not the main issue. The main issue is to warn the user in a explicit, clear way (that there are geometry issues). And personally I prefer to have also the operation stopped rather than having a wrong output that I know that I have to redo again anyway (after cleaning/fixing).

#8 Updated by Nathan Woodrow over 3 years ago

Right ok. That makes sense. Would this be up front before running the tool itself,e.g run valid check, continue if all good. If that was the default action I think that would solve a "I ran this for over and hour and it stopped, now I have to start again" situations.

#9 Updated by Giovanni Manghi over 3 years ago

Nathan Woodrow wrote:

Right ok. That makes sense. Would this be up front before running the tool itself,e.g run valid check, continue if all good. If that was the default action I think that would solve a "I ran this for over and hour and it stopped, now I have to start again" situations.

Actually I think that we have already the solution: make liblwgeom a dependency (is available on osgeo4w and all major linux distros) and add to qgis geoprocessing tools an option to clean input layers beforehand. There should be also an option to snap geometries (to close small gaps/overlaps) exactly as we can do in GRASS when importing with v.in.ogr.

#10 Updated by Alexander Bruy over 3 years ago

Nathan Woodrow wrote:

Is there any alternative? Flag and report at the end?

This is how it works now: invalid geometries skipped and information about them added to Processing log.

Giovanni Manghi wrote:

but it is not really that easy: in this case (but also others, I'm putting together a report on the "union" tool) ignoring the geometry error produces a wrong result!

In some cases, e.g. buffering skipping invalid geometries is ok.

Giovanni Manghi wrote:

The main issue is to warn the user in a explicit, clear way (that there are geometry issues).

Another possible approach is to show error message in the algorithm dialog and skip them. In this case user will be warned in "explicit, clear way" and algorithm will continue to work.

Giovanni Manghi wrote:

Actually I think that we have already the solution: make liblwgeom a dependency (is available on osgeo4w and all major linux distros) and add to qgis geoprocessing tools an option to clean input layers beforehand.

-1 from me. First of all this will increase processing time, moreover, moreover many algs does not really need geometry fixing, e.g. merging vector layers, selecting features etc. Also sometimes automatic cleaning may produce wrong results. IMHO better to leave this to users: there are already algs in Processing toolbox to clean geometry (for example, v.clean, (p)prepair), so users can easily run them before main operation, and even create pre-processing script or models if they need/want to run such tools automatically.

#11 Updated by Anita Graser over 3 years ago

Alexander Bruy wrote:

Giovanni Manghi wrote:

Actually I think that we have already the solution: make liblwgeom a dependency (is available on osgeo4w and all major linux distros) and add to qgis geoprocessing tools an option to clean input layers beforehand.

-1 from me. First of all this will increase processing time, moreover, moreover many algs does not really need geometry fixing, e.g. merging vector layers, selecting features etc. Also sometimes automatic cleaning may produce wrong results. IMHO better to leave this to users

Imho, if it's an option, which is off by default, a "try to automatically clean my input layers" option could be helpful.

#12 Updated by Giovanni Manghi over 3 years ago

Imho, if it's an option, which is off by default, a "try to automatically clean my input layers" option could be helpful.

yes.

Anyway we can discuss this later.

Now it is paramount to (finally) have qgis geoprocessing tools (aka ftools) stop returning wrong results in an almost completely unnoticeable way.

So... warn the user clearly that there are invalid geometries and the result likely to be wrong and/or stop processing at all.

#13 Updated by Anonymous over 3 years ago

  • Status changed from Open to Closed

#14 Updated by Matthias Kuhn over 3 years ago

The fix adds a new flag "ignore invalid geometries" which if off by default.

The algorithm will abort and inform the user about the invalid geometries and ask him to either

  • fix the geometries
  • specify the "ignore invalid geometries" flag

#15 Updated by Giovanni Manghi over 3 years ago

Matthias Kuhn wrote:

The fix adds a new flag "ignore invalid geometries" which if off by default.

The algorithm will abort and inform the user about the invalid geometries and ask him to either

  • fix the geometries
  • specify the "ignore invalid geometries" flag

Thanks Matthias for tackling this painfully long standing issues!

I believe that the same approach should be applied also to the other geoprocessing operations (union, sym difference, intersection, possibily also clip). Make sense?

#16 Updated by Giovanni Manghi almost 3 years ago

  • Target version deleted (Version 2.16)
  • Status changed from Closed to Reopened
  • Affected QGIS version changed from master to 2.18.0

Matthias Kuhn wrote:

The fix adds a new flag "ignore invalid geometries" which if off by default.

The algorithm will abort and inform the user about the invalid geometries and ask him to either

  • fix the geometries
  • specify the "ignore invalid geometries" flag

Hi Matthias, Alexander, Victor and everyone else, this really should be applied to all tools, otherwise we will also get reports like #15962

I add that it seems about time to have all this QGIS geoprocessing tools fixed once for all, preferably by adding also a way to allow the users to fix the geometries on the fly while running the operations.

#17 Updated by Giovanni Manghi almost 3 years ago

  • Category changed from 44 to Processing/QGIS

#18 Updated by Alexander Bruy over 2 years ago

  • Status changed from Reopened to Feedback

In master it is possible to configure Processing behavior for invalid geometries:
- don't check for geometries validity
- stop algorithm execution if invalid geometry found
- ignore invalid geometries

So I think we can close this issue.

#19 Updated by Giovanni Manghi over 2 years ago

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

Also available in: Atom PDF