Bug fixed in isisminer to properly handle self-intersecting polygon geometries. #5620

kledmundson · 2024-09-23T23:50:12Z

Description

The isisminer application detects bad polygons, reports them, and terminates instead of attempting to repair them. Often (though not always), self-intersecting polygons can be repaired by creating a buffer of size 0 around the polygon.

Here, the Strategy and GisGeometry classes have been updated to better manage geometries. The buffer(0) method was added to GisGeometry to repair self-intersecting polygons. The RepairInvalidGeometry and InvalidGeometryAction keywords have been added to the Strategy class providing improved user control over invalid geometries.

The RepairInvalidGeometry keyword defaults to true so that invalid geometries will be repaired. Options for the InvalidGeometryAction
keyword are continue, disable or error. For InvalidGeometryAction = continue, the state of the invalid geometry is retained in the Resource (some GIS operations still function) and it is not disabled and no error occurs - the issue is ignored. If InvalidGeometryAction = disable, then the geometry is retained in the Resource but the status is set to "discard". It can be re-enabled by using the ResourceManager strategy. If InvalidGeometryAction = error, then an error is thrown and isisminer terminates. The default value is disable.

Related Issue

https://github.com//issues/5612

How Has This Been Validated?

The GisGeometry unit test has been modified to test the validity and repair of a self-intersecting polygon. The Strategy unit test has been updated to test the behavior of the two new keywords RepairInvalidGeometry and InvalidGeometryAction.

GisGeometry unit test result

ctest -R GisGeometry --output-on-failure
Start 108: isis_unit_test_GisGeometry
1/1 Test #108: isis_unit_test_GisGeometry ....... Passed 2.01 sec

Strategy unit test result

ctest -R isis_unit_test_Strategy --output-on-failure
Start 299: isis_unit_test_Strategy
1/1 Test #299: isis_unit_test_Strategy .......... Passed 0.56 sec

Additionally, all isisminer app tests continue to pass.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Documentation change (update to the documentation; no code change)
Breaking change (fix or feature that would cause existing functionality to change)
Infrastructure change (changes to things like CI or the build system that do not impact users)

Checklist:

I have read and agree to abide by the Code of Conduct
I have read the CONTRIBUTING document.
My change requires a change to the documentation and I have updated the documentation accordingly.
I have added tests to cover my changes.
I have added myself to the .zenodo.json document.
I have added my user impacting change to the CHANGELOG.md document.

Licensing

This project is mostly composed of free and unencumbered software released into the public domain, and we are unlikely to accept contributions that are not also released into the public domain. Somewhere near the top of each file should have these words:

This work is free and unencumbered software released into the public domain. In jurisdictions that recognize copyright laws, the author or authors of this software dedicate any and all copyright interest in the software to the public domain.

I dedicate any and all copyright interest in this software to the public domain. I make this dedication for the benefit of the public at large and to the detriment of my heirs and successors. I intend this dedication to be an overt act of relinquishment in perpetuity of all present and future rights to this software under copyright law.

github-actions · 2024-09-23T23:50:55Z

The build and test suite have started for your pull request.

To view your build log, please reference the build with source version: "PR_5620".

Additionally, check the latest "dev" source version to identify existing test failures. Please note that you are not responsible for the test failures that exist on both your PR and the dev branch.

AustinSanders · 2024-10-04T20:44:17Z

isis/src/base/objs/GisGeometry/GisGeometry.cpp

+    int valid(0);
+    try {
+        valid = GEOSisValid(this->m_geom);
+    } catch (...) {


If I'm reading the docs correctly, the catch(...) is not necessary / reachable.

It looks like exceptions are handled within GEOSisValid, and the function returns 2 on exception.

isis/src/base/objs/GisGeometry/GisGeometry.cpp

github-actions · 2024-10-07T21:23:32Z

The build and test suite have started for your pull request.

To view your build log, please reference the build with source version: "PR_5620".

Additionally, check the latest "dev" source version to identify existing test failures. Please note that you are not responsible for the test failures that exist on both your PR and the dev branch.

* GisGeometry was throwing an exception when isValid() was called and the geometry was indeed invalid * GisGeometry - added buffer() method * Strategy was updated to better manage geometries * Strategy - Added RepairInvalidGeometry and InvalidGeometryAction to allow better user control over invalid geometries * Strategy - Apply buffer(0) algorithm when an invalid geometry is detected * Strategy - Added more debug output

…5612. * GisGeometry was throwing an exception when isValid() was called and the geometry was indeed invalid * GisGeometry - added buffer() method * Strategy was updated to better manage geometries * Strategy - Added RepairInvalidGeometry and InvalidGeometryAction to allow better user control over invalid geometries * Strategy - Apply buffer(0) algorithm when an invalid geometry is detected * Strategy - Added more debug output * isisminer - improved handling of invalid/bad geometries * isisminer - Documented new parameters RepairInvalidGeometry and InvalidGeometryAction and updated Calculator strategy documentation

…n geometries were not treated properly. Added pertinent unit tests to GisGeometry and Strategy classes. Fixed incorrect links and minor typos in isisminer documentation. Addresses DOI-USGS#5612.

… Addresses DOI-USGS#5612.

github-actions · 2024-10-08T17:39:17Z

The build and test suite have started for your pull request.

To view your build log, please reference the build with source version: "PR_5620".

Additionally, check the latest "dev" source version to identify existing test failures. Please note that you are not responsible for the test failures that exist on both your PR and the dev branch.

github-actions · 2024-10-08T17:39:24Z

The build and test suite have started for your pull request.

To view your build log, please reference the build with source version: "PR_5620".

Additionally, check the latest "dev" source version to identify existing test failures. Please note that you are not responsible for the test failures that exist on both your PR and the dev branch.

kledmundson · 2024-10-08T17:43:08Z

Thanks for reviewing @AustinSanders. I fixed the typo you caught (plus a couple more). Regarding the method GisGeometry::isValid(), we have confirmed that it is correct as written. Verified that in the presence of self-intersecting polygon geometry the catch(...) statement is executed properly.

AustinSanders

I'm not understanding what types of exception can be thrown by GeosIsValid(), which is leading to my confusion about the catch(...). The documentation states that the function returns "1 on true, 0 on false, 2 on exception."

Ultimately, I trust that the code is functioning properly, I just don't understand the circumstances under which the catch(...) is necessary. I'll approve for merge, but I'd also like to understand the code better if you don't mind discussing.

kledmundson · 2024-10-15T19:36:38Z

Ok, thanks. What's going on with this does appear to be a bit cryptic to me as well, considering the GEOS documentation. I will rerun again with & without the try/catch in the presence of self-intersecting polygons and provide more details.

…

On Tue, Oct 15, 2024 at 12:30 PM Austin Sanders ***@***.***> wrote: ***@***.**** approved this pull request. I'm not understanding what types of exception can be thrown by GeosIsValid(), which is leading to my confusion about the catch(...). The documentation states that the function returns "1 on true, 0 on false, 2 on exception." Ultimately, I trust that the code is functioning properly, I just don't understand the circumstances under which the catch(...) is necessary. I'll approve for merge, but I'd also like to understand the code better if you don't mind discussing. — Reply to this email directly, view it on GitHub <#5620 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABUGSUUSJ5Y4XE66H6XNY3TZ3VUNBAVCNFSM6AAAAABOXDIJHOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGNZQGM3TIMJZGI> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

kledmundson · 2024-10-17T15:01:52Z

Hi @AustinSanders, Kris and I went through this yesterday, here are the details (let us know what you think) ...

The GEOS C-API requires that users of the library provide handlers for both GEOS notices and errors. Static methods implemented in the GisTopology class receive and process status messages in text. GisTopology::notice() and GisTopology:error() provide GOES/ISIS notice/error handling.
The try/catch structure in GisGeometry:: isValid() is required as GEOS notices and errors are relayed back to GisTopology::notice() and GisTopology:error() respectively. They both throw ISIS exceptions when they are called from within the GEOS system when it generates an internal notice or error. See also GisTopology::geosInit() which initializes this condition upon factory creation.
A GEOS notice is generated when evaluation of any GEOSGeometry contains a self-intersection. This, in turn, results in an ISIS exception, which is thrown in GisTopology::notice(), even when GEOSisValid() is called. Below is the output generated from isis_unit_test_GisGeometry when we replaced the throw with a print stmt. Note that GisGeometry::isValidReason() can be called after a false return from GisGeometry::isValid() to retrieve that same message.

< GEOS Notice: Self-intersection at or near point 286.74713584288054 51.271685761047472 91d89
< GEOS Notice: Self-intersection at or near point 286.74713584288054 51.271685761047472 94d91
< GEOS Notice: Self-intersection at or near point 286.74713584288054 51.271685761047472 96d92
< GEOS Notice: Self-intersection at or near point 286.74713584288054 51.271685761047472 98d93
< GEOS Notice: Self-intersection at or near point 286.74713584288054 51.271685761047472

We think it might be ok to remove the throw in GisTopology::notice() because the desired behavior is retained without the try/catch. However, we would be reluctant to remove the throw in GisTopology:error() as GEOS error behavior is uncertain (e.g., GEOSisValid() may be the only opportunity to handle real GEOS errors).
While it's true that the GEOS documentation indicates that GeosIsValid() will not produce an error, the above design/implementation decisions made the catch(...) necessary.

AustinSanders · 2024-10-17T15:54:55Z

That all makes sense! Thanks for writing that up

…eometries. (DOI-USGS#5620) * Fixed issues with GisGeometry/Strategy classes. Addresses DOI-USGS#5612. * GisGeometry was throwing an exception when isValid() was called and the geometry was indeed invalid * GisGeometry - added buffer() method * Strategy was updated to better manage geometries * Strategy - Added RepairInvalidGeometry and InvalidGeometryAction to allow better user control over invalid geometries * Strategy - Apply buffer(0) algorithm when an invalid geometry is detected * Strategy - Added more debug output * Corrected invalid geometry problems in isisminer. Addresses DOI-USGS#5612. * GisGeometry was throwing an exception when isValid() was called and the geometry was indeed invalid * GisGeometry - added buffer() method * Strategy was updated to better manage geometries * Strategy - Added RepairInvalidGeometry and InvalidGeometryAction to allow better user control over invalid geometries * Strategy - Apply buffer(0) algorithm when an invalid geometry is detected * Strategy - Added more debug output * isisminer - improved handling of invalid/bad geometries * isisminer - Documented new parameters RepairInvalidGeometry and InvalidGeometryAction and updated Calculator strategy documentation * Fixed a bug in isisminer in which bad (e.g. self-intersecting) polygon geometries were not treated properly. Added pertinent unit tests to GisGeometry and Strategy classes. Fixed incorrect links and minor typos in isisminer documentation. Addresses DOI-USGS#5612. * Per reviewer comments, corrected additional typos in GisGeometry.cpp. Addresses DOI-USGS#5612. --------- Co-authored-by: Kris J. Becker <[email protected]>

acpaquette mentioned this pull request Sep 24, 2024

Detect and Fix Bad GIS Footprints in isisminer #3601

Closed

Kelvinrr assigned AustinSanders Oct 4, 2024

AustinSanders suggested changes Oct 7, 2024

View reviewed changes

kledmundson closed this Oct 7, 2024

kledmundson force-pushed the bugfix/isisminerSelfIntersectingGeometry branch from 38b1cc2 to f829674 Compare October 7, 2024 21:22

kledmundson deleted the bugfix/isisminerSelfIntersectingGeometry branch October 8, 2024 16:22

KrisBecker and others added 4 commits October 8, 2024 09:51

Fixed a bug in isisminer in which bad (e.g. self-intersecting) polygo…

f2942db

…n geometries were not treated properly. Added pertinent unit tests to GisGeometry and Strategy classes. Fixed incorrect links and minor typos in isisminer documentation. Addresses DOI-USGS#5612.

Per reviewer comments, corrected additional typos in GisGeometry.cpp.…

0bac3de

… Addresses DOI-USGS#5612.

kledmundson reopened this Oct 8, 2024

acpaquette requested a review from AustinSanders October 8, 2024 23:59

AustinSanders approved these changes Oct 15, 2024

View reviewed changes

AustinSanders merged commit 7ca57fa into DOI-USGS:dev Oct 17, 2024
4 checks passed

kledmundson mentioned this pull request Oct 17, 2024

Isisminer does not properly handle the presence of bad (e.g. self-intersecting) polygons. #5612

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fixed in isisminer to properly handle self-intersecting polygon geometries. #5620

Bug fixed in isisminer to properly handle self-intersecting polygon geometries. #5620

kledmundson commented Sep 23, 2024

github-actions bot commented Sep 23, 2024

AustinSanders Oct 4, 2024

github-actions bot commented Oct 7, 2024

github-actions bot commented Oct 8, 2024

github-actions bot commented Oct 8, 2024

kledmundson commented Oct 8, 2024 •

edited

Loading

AustinSanders left a comment

kledmundson commented Oct 15, 2024 via email

kledmundson commented Oct 17, 2024

AustinSanders commented Oct 17, 2024

Bug fixed in isisminer to properly handle self-intersecting polygon geometries. #5620

Bug fixed in isisminer to properly handle self-intersecting polygon geometries. #5620

Conversation

kledmundson commented Sep 23, 2024

Description

Related Issue

How Has This Been Validated?

Types of changes

Checklist:

Licensing

github-actions bot commented Sep 23, 2024

AustinSanders Oct 4, 2024

Choose a reason for hiding this comment

github-actions bot commented Oct 7, 2024

github-actions bot commented Oct 8, 2024

github-actions bot commented Oct 8, 2024

kledmundson commented Oct 8, 2024 • edited Loading

AustinSanders left a comment

Choose a reason for hiding this comment

kledmundson commented Oct 15, 2024 via email

kledmundson commented Oct 17, 2024

AustinSanders commented Oct 17, 2024

kledmundson commented Oct 8, 2024 •

edited

Loading