Optional densify simplify #9

oscarfasanchez · 2025-12-08T09:07:24Z

This pull request adds geometry simplification and densification options to the conceptual mesh blueprint, allowing users to control the simplification of polygons, lines, and points, as well as providing more flexible densification for lines. It also introduces tests to verify these new features. The most important changes are grouped below.

Geometry Simplification Features:

Added a simplify_tolerance parameter to the add_polygon, add_line, and add_point methods in blueprint.py, enabling optional geometry simplification for each feature type. The simplification is applied before overlap resolution and mesh generation.
Implemented the _apply_simplification method to handle geometry simplification for polygons and lines, and to deduplicate points that are within the specified tolerance, keeping the point with the finest resolution.
Updated the mesh generation pipeline to call _apply_simplification before resolving overlaps.
Ensured that empty polygon lists result in an empty GeoDataFrame with all expected columns, improving robustness.

Line Densification Enhancements:

Extended the add_line method to accept a densify parameter, which can be True, False, or a float for custom spacing. Updated _apply_densification to respect this parameter, allowing for more flexible control over line densification.

Testing Improvements:

Added new tests in test_conceptual_mesh.py to verify:
- Polygon simplification removes small geometric noise.
- Line simplification reduces unnecessary vertices.
- Point deduplication merges close points and keeps the one with the finest resolution.
- Line densification works in all three modes (False, True, and custom float).

…ut still not implemented the last one

… desired

…nto optional_densify_simplify

Copilot

Pull request overview

This pull request enhances the conceptual mesh blueprint with optional geometry simplification and flexible line densification capabilities. The changes enable users to reduce geometric complexity before mesh generation while maintaining control over simplification tolerances and densification strategies.

Added simplify_tolerance parameter to polygons, lines, and points for optional geometry simplification
Extended line densification to support three modes: disabled (False), automatic (using resolution), or custom spacing (float value)
Implemented point deduplication that merges nearby points within tolerance, keeping the point with finest resolution
Added comprehensive tests to verify polygon/line simplification, point deduplication, and all line densification modes

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 15 comments.

File	Description
src/vorflow/blueprint.py	Core implementation of simplification and densification features, including new parameters in `add_polygon`, `add_line`, and `add_point` methods, plus the new `_apply_simplification` method and enhanced `_apply_densification` logic
tests/test_conceptual_mesh.py	Comprehensive test coverage for new features including polygon/line simplification, point deduplication with resolution preservation, and three modes of line densification

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-08T12:40:07Z

src/vorflow/blueprint.py

+                lambda row: self._densify_geometry(row['geometry'], get_line_resolution(row))
+                if get_line_resolution(row) is not None else row['geometry'], axis=1)


The get_line_resolution function is called twice per row in the lambda (lines 418-419): once to determine the resolution value and once to check if it's None. This is inefficient. Consider calculating it once and reusing the value, or restructuring the apply logic.

Suggested change

lambda row: self._densify_geometry(row['geometry'], get_line_resolution(row))

if get_line_resolution(row) is not None else row['geometry'], axis=1)

lambda row: (

self._densify_geometry(row['geometry'], resolution)

if resolution is not None else row['geometry']

),

axis=1

)

# Use a local variable to avoid double evaluation

# (This is done by defining the lambda with a local variable)

# Alternatively, use a regular function:

# def densify_row(row):

# resolution = get_line_resolution(row)

# return self._densify_geometry(row['geometry'], resolution) if resolution is not None else row['geometry']

# self.clean_lines['geometry'] = self.clean_lines.apply(densify_row, axis=1)

Copilot · 2025-12-08T12:40:07Z

src/vorflow/blueprint.py

+            if tol is True:
+                lc = poly_data.get('lc')
+                tol = lc * 0.5 if lc is not None else None


The logic for handling tol is True (lines 145-147, 155-157, 179-181) is present but not documented in the API. The simplify_tolerance parameter is documented as float, optional in the docstrings, but the code accepts True as a special value that computes a tolerance based on resolution. Either document this behavior in the parameter descriptions or remove support for True if it's not intended to be part of the public API.

Copilot · 2025-12-08T12:40:08Z

src/vorflow/blueprint.py

+            #lets sort by resolution first
+            sorted_points = sorted(
+                self.raw_points,
+                  key=lambda x: x['lc'] if x['lc'] is not None else float('inf'))


Inconsistent indentation: the sorted_points tuple has improper alignment with 'key=' on line 167 indented more than expected. This should align with the opening parenthesis on line 165.

Suggested change

key=lambda x: x['lc'] if x['lc'] is not None else float('inf'))

key=lambda x: x['lc'] if x['lc'] is not None else float('inf'))

Copilot · 2025-12-08T12:40:08Z

src/vorflow/blueprint.py

+                for kept in final_points:
+                    dist = kept['geometry'].distance(current_geom)
+                    if dist < tol:
+                        is_merged = True
+                        break
+                if not is_merged:
+                    final_points.append(point_data)


The point deduplication algorithm has O(n²) complexity because it compares each point against all previously kept points (line 184-188). For large numbers of points, this could be a performance bottleneck. Consider using a spatial index (e.g., STRtree from shapely) to efficiently find nearby points within the tolerance distance.

Copilot · 2025-12-08T12:40:08Z

src/vorflow/blueprint.py

+                tol = lc * 0.5 if lc is not None else None
+
+            if tol is not None and tol > 0:
+                self.raw_polygons[i]['geometry'] = poly_data['geometry'].simplify(tol, preserve_topology=False)


Using preserve_topology=False for polygon simplification could create invalid geometries or self-intersections. This is risky for mesh generation. Consider using preserve_topology=True to maintain topological validity, or validate the geometry after simplification.

Suggested change

self.raw_polygons[i]['geometry'] = poly_data['geometry'].simplify(tol, preserve_topology=False)

self.raw_polygons[i]['geometry'] = poly_data['geometry'].simplify(tol, preserve_topology=True)

Copilot · 2025-12-08T12:40:10Z

src/vorflow/blueprint.py

        lower ones.
        """
        # Sort polygons by priority, with the highest z_order processed first.
+        if self.raw_polygons == []:


The empty list comparison self.raw_polygons == [] is not the most Pythonic way to check for emptiness. Use not self.raw_polygons instead, which is the idiomatic way to check for empty lists in Python.

Suggested change

if self.raw_polygons == []:

if not self.raw_polygons:

Copilot · 2025-12-08T12:40:10Z

src/vorflow/blueprint.py

        """
        # Sort polygons by priority, with the highest z_order processed first.
+        if self.raw_polygons == []:
+            #if this is empty, just create an empty GeoDataFrame


The comment on line 207 says "if this is empty, just create an empty GeoDataFrame" but should start with a capital letter and use proper punctuation: "If this is empty, just create an empty GeoDataFrame."

Suggested change

#if this is empty, just create an empty GeoDataFrame

# If this is empty, just create an empty GeoDataFrame.

Copilot · 2025-12-08T12:40:10Z

src/vorflow/blueprint.py

+    def _apply_simplification(self):
+        """
+        Applies geometry simplification to raw polygons, lines, and points
+        based on their specified tolerances.


The _apply_simplification() method lacks a docstring describing the point deduplication behavior and algorithm. The current docstring only mentions "simplification" but doesn't explain that points are deduplicated by merging those within tolerance, keeping the one with finest resolution.

Suggested change

based on their specified tolerances.

based on their specified tolerances.

For points, this method also performs deduplication: points that are within

a specified tolerance of each other are merged, and only the point with the

finest (smallest) resolution is kept. The tolerance for merging is taken from

the 'simplify_tolerance' attribute of each point, or derived from the point's

resolution if set to True. This ensures that closely spaced points do not

result in redundant mesh nodes, and that the most restrictive mesh size is

preserved at each location.

Copilot · 2025-12-08T12:40:11Z

src/vorflow/blueprint.py

+        # lets merge points that are very close to each other
+        if self.raw_points:
+            #lets sort by resolution first


Comment style issues: Line 162 should start with a capital letter and use proper grammar ("Let's merge" instead of "lets merge"). Line 164 is missing a space after '#' (should be "# Let's sort" instead of "#lets sort").

Suggested change

# lets merge points that are very close to each other

if self.raw_points:

#lets sort by resolution first

# Let's merge points that are very close to each other

if self.raw_points:

# Let's sort by resolution first

Copilot · 2025-12-08T12:40:11Z

tests/test_conceptual_mesh.py

+    poly = Polygon([
+        (0, 0), (1, 0), 
+        (1, 1), (0.5, 1.001), (0, 1)
+    ])


[nitpick] Inconsistent spacing in line continuation. The closing parenthesis on line 62 is at a different indentation level than the opening on line 59. For better readability, align the closing parenthesis with the opening statement or place it at the same indentation as the content.

rhugman · 2025-12-08T13:20:10Z

@oscarfasanchez - Id recommend adding logging of how much change is incurred in the geometries through simplification. A warning message should inform the user that simplification has resulted in e.g. "x% area difference". Things like that.

…nd simplify

oscarfasanchez · 2025-12-09T08:40:53Z

I have committed the recommendations made by @rhugman and Copilot

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-09T08:47:12Z

src/vorflow/blueprint.py

+                    reduction_pct = 100 * (org_area - new_area) / org_area
+                    if reduction_pct > 1.0:


The threshold value of 1.0% for reporting area reduction is a magic number. Consider making it a named constant (e.g., SIGNIFICANT_REDUCTION_PCT = 1.0) to improve code maintainability and make it easier to adjust this threshold in the future.

Copilot · 2025-12-09T08:47:12Z

src/vorflow/blueprint.py

+            tol = poly_data.get('simplify_tolerance')
+            if tol is True:
+                lc = poly_data.get('lc')
+                tol = lc * 0.5 if lc is not None else None


The heuristic multiplier of 0.5 for automatic tolerance calculation (when simplify_tolerance=True) is a magic number. Consider defining it as a named constant (e.g., AUTO_SIMPLIFY_FACTOR = 0.5) to make it easier to adjust and maintain consistency across polygon, line, and point simplification.

Copilot · 2025-12-09T08:47:12Z

src/vorflow/blueprint.py

+            tol = line_data.get('simplify_tolerance')
+            if tol is True:
+                lc = line_data.get('lc')
+                tol = lc * 0.5 if lc is not None else None


The heuristic multiplier of 0.5 for automatic tolerance calculation is duplicated from line 180. Consider defining it as a named constant to avoid duplication and ensure consistency.

Copilot · 2025-12-09T08:47:12Z

src/vorflow/blueprint.py

+
+                if tol is True:
+                    lc = point_data.get('lc')
+                    tol = lc * 0.5 if lc is not None else 1e-6


The fallback tolerance value of 1e-6 and the automatic tolerance multiplier of 0.5 are magic numbers. These values are also used elsewhere in the code. Consider defining them as named constants for consistency and maintainability.

Copilot · 2025-12-09T08:47:13Z

src/vorflow/blueprint.py

+        """
+        Applies geometry simplification to raw polygons, lines, and points
+        based on their specified tolerances to reduce
+        geometric complexity.For polygon and lines


Missing space after period in docstring. Should be "...complexity. For polygon and lines..." instead of "...complexity.For polygon and lines..."

Suggested change

geometric complexity.For polygon and lines

geometric complexity. For polygon and lines

Copilot · 2025-12-09T08:47:13Z

src/vorflow/blueprint.py

+                    return None
+
+                # 2. Explicit custom resolution (e.g., densify=5.0)
+                if isinstance(d, (int, float)) and d > 0:


The validation logic has the same issue as the validation in add_line: booleans are instances of int in Python, so isinstance(d, (int, float)) will be True when d is True or False. This could cause densify=True to be interpreted as the numeric value 1 instead of the intended default behavior. The check should exclude booleans: if isinstance(d, (int, float)) and not isinstance(d, bool) and d > 0:

Suggested change

if isinstance(d, (int, float)) and d > 0:

if isinstance(d, (int, float)) and not isinstance(d, bool) and d > 0:

Copilot · 2025-12-09T08:47:13Z

src/vorflow/blueprint.py

+        """
+        Applies geometry simplification to raw polygons, lines, and points
+        based on their specified tolerances to reduce
+        geometric complexity.For polygon and lines


The docstring states "For polygon and lines" but should be "For polygons and lines" (polygon should be plural to match "lines").

Suggested change

geometric complexity.For polygon and lines

geometric complexity. For polygons and lines

Copilot · 2025-12-09T08:47:14Z

src/vorflow/blueprint.py

+                    tol = lc * 0.5 if lc is not None else 1e-6
+                is_merged = False
+
+                #query tree for potential neighbors


Comment should start with a capital letter: "Query tree for potential neighbors" instead of "query tree for potential neighbors"

Suggested change

#query tree for potential neighbors

# Query tree for potential neighbors

…et of variables for magic numbers

oscarfasanchez · 2025-12-10T03:18:03Z

Solved.

rhugman · 2025-12-10T12:06:02Z

@oscarfasanchez

.add_polygon() and .add_line() now have different arguments that do the same thing. I think we should merge 'border_densityanddensify. Just keep densify`.
simplify_tolerance=True is pretty aggressive. I think this option is misleading. AUTO_SIMPLIFY_FACTOR should probalby be somthing like 1%? Half cell size shift seems a bit harsh.
a significant difference between this approach and the utils.resample_geometry, is that you are relying on the shapely .simplify() to reduce unecessary node spacing, then addiign new points to make sure spacing is not too large (fair enough). resample_geometry() is actualy resampling the whole geometry uniformly. It also ensures that polygons are note degenerate (i.e., very small slivers are cleaned up). So slightly different use cases.
this is an issue: something has changed in regard to how resolution in holes in overlapping polygons is handled. Before the PR, holes would get the resolution of the underlying poly. Now Somethign very weird is happening wher eit seems we get the higher esolution imposed onto the coarser background resolution (...words...see screenshot below).

oscarfasanchez added 4 commits December 8, 2025 15:01

added optional simplification and densification argument for lines, b…

745b238

…ut still not implemented the last one

added option for no densification of lines or custom densification if…

decd562

… desired

added test and debugged the tests, fixed bug on line implementation

28d8324

Merge branch 'main' of https://github.com/oscarfasanchez/vorflow_os i…

4a39c8e

…nto optional_densify_simplify

rhugman requested a review from Copilot December 8, 2025 12:33

Copilot started reviewing on behalf of rhugman December 8, 2025 12:33 View session

Copilot AI reviewed Dec 8, 2025

View reviewed changes

added fixes and recommendations to pull request of optional densify a…

145bd21

…nd simplify

oscarfasanchez requested a review from Copilot December 9, 2025 08:41

Copilot started reviewing on behalf of oscarfasanchez December 9, 2025 08:41 View session

Copilot AI reviewed Dec 9, 2025

View reviewed changes

adding second round of refinements from copilot, mainly grammar and s…

a848493

…et of variables for magic numbers

deleted the simplify default and standardized the densify keyword

728765d

		lambda row: self._densify_geometry(row['geometry'], get_line_resolution(row))
		if get_line_resolution(row) is not None else row['geometry'], axis=1)

-                lambda row: self._densify_geometry(row['geometry'], get_line_resolution(row))
-                if get_line_resolution(row) is not None else row['geometry'], axis=1)
+                lambda row: (
+                    self._densify_geometry(row['geometry'], resolution)
+                    if resolution is not None else row['geometry']
+                ),
+                axis=1
+            )
+            # Use a local variable to avoid double evaluation
+            # (This is done by defining the lambda with a local variable)
+            # Alternatively, use a regular function:
+            # def densify_row(row):
+            #     resolution = get_line_resolution(row)
+            #     return self._densify_geometry(row['geometry'], resolution) if resolution is not None else row['geometry']
+            # self.clean_lines['geometry'] = self.clean_lines.apply(densify_row, axis=1)

	key=lambda x: x['lc'] if x['lc'] is not None else float('inf'))
	key=lambda x: x['lc'] if x['lc'] is not None else float('inf'))

	self.raw_polygons[i]['geometry'] = poly_data['geometry'].simplify(tol, preserve_topology=False)
	self.raw_polygons[i]['geometry'] = poly_data['geometry'].simplify(tol, preserve_topology=True)

	#if this is empty, just create an empty GeoDataFrame
	# If this is empty, just create an empty GeoDataFrame.

-        based on their specified tolerances.
+        based on their specified tolerances.
+        For points, this method also performs deduplication: points that are within
+        a specified tolerance of each other are merged, and only the point with the
+        finest (smallest) resolution is kept. The tolerance for merging is taken from
+        the 'simplify_tolerance' attribute of each point, or derived from the point's
+        resolution if set to True. This ensures that closely spaced points do not
+        result in redundant mesh nodes, and that the most restrictive mesh size is
+        preserved at each location.

		reduction_pct = 100 * (org_area - new_area) / org_area
		if reduction_pct > 1.0:

	geometric complexity.For polygon and lines
	geometric complexity. For polygon and lines

	if isinstance(d, (int, float)) and d > 0:
	if isinstance(d, (int, float)) and not isinstance(d, bool) and d > 0:

	geometric complexity.For polygon and lines
	geometric complexity. For polygons and lines

	#query tree for potential neighbors
	# Query tree for potential neighbors

Optional densify simplify #9

Are you sure you want to change the base?

Optional densify simplify #9

Uh oh!

Conversation

oscarfasanchez commented Dec 8, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

rhugman commented Dec 8, 2025

Uh oh!

oscarfasanchez commented Dec 9, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

oscarfasanchez commented Dec 10, 2025

Uh oh!

rhugman commented Dec 10, 2025

Uh oh!

Reviewers

Assignees