Remove small parts

[gluser1357]

Pull request for enhancement request https://trac.osgeo.org/postgis/ticket/5706
See details there, thanks.

Summary by CodeRabbit

• New Features

  • Introduced the ST_RemoveSmallParts function to remove small parts from geometries in 2D polygons and polylines, enhancing the precision and usability of geometric data handling.

• Documentation

  • Added detailed documentation for the ST_RemoveSmallParts function, including behavior explanations and examples for different geometry types.

• Tests

  • Implemented new tests to validate the functionality of the ST_RemoveSmallParts function with various geometries and parameters.

[robe2]

@gluser1357 Looks like it's failing on our bots treating warnings as errors. Can you fix:

https://github.com/postgis/postgis/actions/runs/8474473891/job/23259878295?pr=767

lwgeom_remove_small_parts.c: In function 'ST_RemoveSmallParts':
lwgeom_remove_small_parts.c:148:20: error: comparison of integer expressions of different signedness: 'int' and 'uint32_t' {aka 'unsigned int'} [-Werror=sign-compare]
  148 |         for (i=0; i<mline->ngeoms; i++) {
      |                    ^
lwgeom_remove_small_parts.c:168:28: error: comparison of integer expressions of different signedness: 'int' and 'uint32_t' {aka 'unsigned int'} [-Werror=sign-compare]
  168 |                 for (i=0; i<polygon->nrings; i++) {
      |                            ^
lwgeom_remove_small_parts.c:179:48: error: comparison of integer expressions of different signedness: 'int' and 'uint32_t' {aka 'unsigned int'} [-Werror=sign-compare]
  179 |                                     for (k=0; k<polygon->nrings; k++) {
      |                                                ^
lwgeom_remove_small_parts.c:197:28: error: comparison of integer expressions of different signedness: 'int' and 'uint32_t' {aka 'unsigned int'} [-Werror=sign-compare]
  197 |                 for (j=0; j<mpolygon->ngeoms; j++) {
      |                            ^
lwgeom_remove_small_parts.c:201:32: error: comparison of integer expressions of different signedness: 'int' and 'uint32_t' {aka 'unsigned int'} [-Werror=sign-compare]
  201 |                     for (i=0; i<polygon->nrings; i++) {
      |                                ^
lwgeom_remove_small_parts.c:212:60: error: comparison of integer expressions of different signedness: 'int' and 'uint32_t' {aka 'unsigned int'} [-Werror=sign-compare]
  212 |                                                 for (k=0; k<polygon->nrings; k++) {
      |                                                            ^
cc1: all warnings being treated as errors

[Komzpa]

We needed a similar function in 2015 and 2018 and published it in SQL form here: https://pgxn.org/dist/lostgis/1.0.0/doc/lostgis.html#ST_FilterSmallRings.sql

1. We made sure that it is using ring area as filter target, so that long slivers can be removed. Will it also make sense for your scenario?

2. I see that you pass X and Y dimensions separately, why?

3. Is there actual use case when you want to run it on both lines and polygons at the same time? We only ever needed this for polygons. Lines seem to be well handled by ST_Simplify* family of functions.

[gluser1357]

Looks like it's failing on our bots treating warnings as errors. Can you fix:...

I changed variables from signed to unsigned int but I'm not sure if this fixes the problem. Can I start the build myself, or how can I test it the same way as your test (treat warnings as errors)?

[gluser1357]

@Komzpa: Thank you for your valuable feedback.

We needed a similar function in 2015 and 2018 and published it in SQL form here: https://pgxn.org/dist/lostgis/1.0.0/doc/lostgis.html#ST_FilterSmallRings.sql. We made sure that it is using ring area as filter target, so that long slivers can be removed. Will it also make sense for your scenario?

We use this function in order to sort out small areas given by the number of pixels in x and y dimension. In our scenario, the real size of a geometry does not matter, e. g. a filled circle (with a bigger area size) compared with a donut (with a lower area size) shall be treated the same way although the size is very different.

I actually also thought about using the area size but then the question arose if the "real" area size of the polygon shall matter (exterior minus interior size) or the area size of each ring (positive or negative). For our use case, it was easier to stay with the x and y dimensions.

Since performance does matter I'm not sure if a pure SQL-based function like ST_FilterSmallRings.sql will perform as good as a c-based function. High performance was our main motivation to introduce this function.

We choose the word "parts" in removeSmallParts to let the function be generic for all possible shapes (rings, linestrings, polygons, and possibly also other types later on) although currently only (multi)polygons and (multi)linestring types are supported.

I see that you pass X and Y dimensions separately, why?

We store coordinates in latitude and longitude (EPSG 4326). Without projecting coordinates one could convert pixels to minSizeX and minSizeY where minSizeX can be different from minSizeY for different latitudes (dependent on cosine), allowing to preprocess (=remove small geometry parts) more accurate than assuming minSizeX = minSizeY.

Is there actual use case when you want to run it on both lines and polygons at the same time? We only ever needed this for polygons. Lines seem to be well handled by ST_Simplify* family of functions.

Our idea was to supply one function that can handle all geometry types (polygons, linestrings and later others) and scales (geometry collections, multi-types, polygons, polygon rings and so on) at the same time. But one could limit the function to polygons, yes.

ST_Simplify* might have been introduced for a different purpose, but maybe I overlook something?

[robe2]

Looks like it's failing on our bots treating warnings as errors. Can you fix:...

I changed variables from signed to unsigned int but I'm not sure if this fixes the problem. Can I start the build myself, or how can I test it the same way as your test (treat warnings as errors)?

Okay rerunning now.

[robe2]

Looks like it's failing on our bots treating warnings as errors. Can you fix:...

I changed variables from signed to unsigned int but I'm not sure if this fixes the problem. Can I start the build myself, or how can I test it the same way as your test (treat warnings as errors)?

Okay rerunning now.

okay recent change fixed the signed integer issue

[robe2]

When you get a chance take a look at @lnicola request for changes

[gluser1357]

My latest commits contain just some minor edits (comment and formatting cleanup, removal of irrelevant dimension check the same way as in PR #767).

Thank you for further reviews, remaining in hope that it'll make it into 3.5.0 :)

[Komzpa]

@coderabbitai review

[coderabbitai]

Walkthrough

Walkthrough

The update introduces a new function ST_RemoveSmallParts to PostGIS, aimed at enhancing geometry processing by removing small parts from polygons and linestrings based on specified dimensions. This change spans across documentation, code implementation, and testing, ensuring thorough integration and demonstration of the new functionality in the system.

Changes

Files	Changes
doc/reference_editor.xml postgis/postgis.sql.in	Added documentation and SQL definition for ST_RemoveSmallParts.
postgis/Makefile.in regress/core/tests.mk.in	Updated makefiles to include new object files and test executables.
postgis/lwgeom_remove_small_parts.c	Introduced code to remove small parts from geometries.
regress/core/remove_small_parts.sql regress/core/remove_small_parts_expected	Added test scripts and expected results for the new function.

Poem

🌟🐰✨
Oh, how the geometries did sprawl,
Until ST_RemoveSmallParts took the call.
Tiny lines and polygons, beware,
With minimum size, no longer there.
Celebrate the clean, the neat, the small—
PostGIS 3.5.0 handles them all!
🌟🐰✨

---

Recent Review Details

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 8c30a25 and bb777cb.

Files ignored due to path filters (1)

• doc/html/images/static/st_removesmallparts.png is excluded by !**/*.png

Files selected for processing (7)

• doc/reference_editor.xml (1 hunks)
• postgis/Makefile.in (1 hunks)
• postgis/lwgeom_remove_small_parts.c (1 hunks)
• postgis/postgis.sql.in (1 hunks)
• regress/core/remove_small_parts.sql (1 hunks)
• regress/core/remove_small_parts_expected (1 hunks)
• regress/core/tests.mk.in (1 hunks)

Files not reviewed due to errors (1)

• postgis.sql.in (no review received)

Additional comments not posted (12)

regress/core/remove_small_parts_expected (1)

1-3: The expected results for the SQL tests are correctly formatted and align with the function's intended output.

regress/core/remove_small_parts.sql (1)

1-17: The SQL test cases are well-structured and correctly demonstrate the usage of ST_RemoveSmallParts with appropriate geometries and parameters.

postgis/lwgeom_remove_small_parts.c (1)

36-235: The implementation of ST_RemoveSmallParts and its helper function ptarray_remove_dim_helper are correctly structured and handle various geometry types as intended. Good error handling and input validation are present.

regress/core/tests.mk.in (1)

99-99: The addition of remove_small_parts to the test executables list is correctly done and follows the existing structure and conventions.

postgis/Makefile.in (1)

134-134: The addition of lwgeom_remove_small_parts.o to the PG_OBJS list is correctly done and follows the existing structure and conventions.

doc/reference_editor.xml (7)

1748-1763: Ensure the function signature in the documentation matches the actual implementation.

---

1765-1767: The description should clearly define what constitutes "small parts" in terms of geometry.

---

1771-1772: Clarify the units of minSizeX and minSizeY. Are they in the coordinate system units of the geometry?

---

1773-1780: Consider adding a note about the potential impact on the topology of the geometry after removing small parts.

---

1793-1804: Verify that the image st_removesmallparts.png exists and is correctly linked in the documentation.

---

1813-1825: Ensure the example provided is correct and effectively demonstrates the functionality of ST_RemoveSmallParts.

---

1827-1836: Add more examples to cover different geometry types and edge cases.

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.