Fix region_to_shapely_polygons #662
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideThis PR refactors geometry conversion by centralizing region_to_shapely_polygons in a shared utility, simplifies the Meshwell 3D and cross-section modules with enhanced typing and streamlined examples, and strengthens test coverage by cleaning up Palace tests, adding input validation, and introducing Meshwell integration tests. Entity relationship diagram for region_to_shapely_polygons refactorerDiagram
REGION ||--o{ POLYGON : contains
POLYGON ||--o{ HOLE : has
POLYGON }|--|| SHAPELY_POLYGON : converts_to
SHAPELY_POLYGON }|--|| MULTIPOLYGON : aggregates
Class diagram for the new geometry utility moduleclassDiagram
class geometry {
+region_to_shapely_polygons(region: kf.kdb.Region) MultiPolygon
}
Class diagram for updated get_meshwell_prisms signature and usageclassDiagram
class get_meshwell_prisms {
+component: gf.Component
+layer_stack: gf.technology.LayerStack
+wafer_layer: gf.typings.Layer | None
+wafer_padding: float | None
+name_by: Literal["layer", "material"]
+returns: List[PolyPrism]
}
Class diagram for PolyPrism construction in Meshwell modulesclassDiagram
class PolyPrism {
+polygons: MultiPolygon
+buffers: Dict[float, float]
+physical_name: str
+mesh_order: int
+mesh_bool: bool
+additive: bool
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider extracting the shared region_to_shapely_polygons logic into a common utilities module instead of importing it across plugins to reduce coupling.
- Double-check that centralizing region_to_shapely_polygons hasn’t changed the behavior in get_meshwell_cross_section, especially the expected return type (List[Polygon] vs MultiPolygon) and hole handling.
- In the main demo of get_meshwell_3D you’re writing to the same filenames in a loop, which will overwrite the first run’s files—consider parameterizing those output names or using a temp directory per iteration.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the shared region_to_shapely_polygons logic into a common utilities module instead of importing it across plugins to reduce coupling.
- Double-check that centralizing region_to_shapely_polygons hasn’t changed the behavior in get_meshwell_cross_section, especially the expected return type (List[Polygon] vs MultiPolygon) and hole handling.
- In the __main__ demo of get_meshwell_3D you’re writing to the same filenames in a loop, which will overwrite the first run’s files—consider parameterizing those output names or using a temp directory per iteration.
## Individual Comments
### Comment 1
<location> `gplugins/meshwell/get_meshwell_3D.py:143-150` </location>
<code_context>
- verbosity=10,
- )
+
+ for component in [ge_detector_straight_si_contacts, add_frame]:
+ prisms = get_meshwell_prisms(
+ component=component(),
+ layer_stack=get_layer_stack(sidewall_angle_wg=0),
+ name_by="layer",
+ )
+
+ cad(entities_list=prisms, output_file="meshwell_prisms_3D.xao")
+ mesh(
+ input_file="meshwell_prisms_3D.xao",
+ output_file="meshwell_prisms_3D.msh",
</code_context>
<issue_to_address>
**issue (bug_risk):** Looping over multiple components in the main block increases test coverage but may overwrite output files.
Since both components write to the same output filenames, only the last set of files will be saved. Please update the code to use unique filenames for each component to prevent overwriting.
</issue_to_address>
### Comment 2
<location> `gplugins/meshwell/tests/test_meshwell.py:15-16` </location>
<code_context>
+)
+from shapely.geometry import LineString
+
+@pytest.mark.parametrize("component", [(bend_circular), (add_frame)])
+def test_prisms(component) -> None:
+ prisms = get_meshwell_prisms(
+ component=component(),
</code_context>
<issue_to_address>
**suggestion (testing):** Missing edge case tests for empty or invalid components.
Add tests for empty, invalid, or non-shape-producing components to verify get_meshwell_prisms handles these cases without errors.
</issue_to_address>
### Comment 3
<location> `gplugins/palace/tests/test_palace.py:94-95` </location>
<code_context>
def test_palace_capacitance_simulation_runs(geometry) -> None:
- c = geometry
run_capacitive_simulation_palace(
- c,
+ geometry,
layer_stack=layer_stack,
material_spec=material_spec,
</code_context>
<issue_to_address>
**suggestion (testing):** Tests lack assertions for simulation results.
Add assertions to check that simulation outputs, such as capacitance values or output files, meet expected criteria to improve test coverage and detect silent failures.
Suggested implementation:
```python
import os
def test_palace_capacitance_simulation_runs(geometry) -> None:
# Run simulation and capture output
result = run_capacitive_simulation_palace(
geometry,
layer_stack=layer_stack,
material_spec=material_spec,
mesh_parameters=get_reasonable_mesh_parameters_capacitance(geometry),
)
# Example: result could be a dict with 'capacitance' and 'output_file'
assert isinstance(result, dict), "Simulation result should be a dict"
assert "capacitance" in result, "Result should contain 'capacitance'"
assert isinstance(result["capacitance"], float), "Capacitance should be a float"
assert result["capacitance"] > 0, "Capacitance should be positive"
assert "output_file" in result, "Result should contain 'output_file'"
assert os.path.exists(result["output_file"]), f"Output file {result['output_file']} should exist"
```
If `run_capacitive_simulation_palace` does not currently return a result dict with 'capacitance' and 'output_file', you will need to update its implementation to do so, or adjust the assertions to match the actual output format.
</issue_to_address>
### Comment 4
<location> `gplugins/palace/tests/test_palace.py:102-103` </location>
<code_context>
)
@pytest.mark.parametrize("n_processes", [(1), (2), (4)])
def test_palace_capacitance_simulation_n_processes(geometry, n_processes) -> None:
- c = geometry
run_capacitive_simulation_palace(
</code_context>
<issue_to_address>
**suggestion (testing):** Missing edge case for invalid n_processes values.
Add tests for invalid n_processes values (such as 0, negatives, or non-integers) to verify proper error handling.
Suggested implementation:
```python
@pytest.mark.parametrize("n_processes", [(1), (2), (4)])
```
```python
@pytest.mark.parametrize("invalid_n_processes", [0, -1, -5, 1.5, "two", None])
def test_palace_capacitance_simulation_invalid_n_processes(geometry, invalid_n_processes) -> None:
import pytest
with pytest.raises((ValueError, TypeError)):
run_capacitive_simulation_palace(
geometry,
layer_stack=layer_stack,
material_spec=material_spec,
mesh_parameters=get_reasonable_mesh_parameters_capacitance(geometry),
n_processes=invalid_n_processes,
)
```
- Ensure that `run_capacitive_simulation_palace` accepts an `n_processes` argument and performs validation, raising `ValueError` or `TypeError` for invalid values.
- Make sure `layer_stack`, `material_spec`, and `get_reasonable_mesh_parameters_capacitance` are available in the test context.
</issue_to_address>
### Comment 5
<location> `gplugins/palace/tests/test_palace.py:113-114` </location>
<code_context>
)
@pytest.mark.parametrize("element_order", [(1), (2), (3)])
def test_palace_capacitance_simulation_element_order(geometry, element_order) -> None:
- c = geometry
run_capacitive_simulation_palace(
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for invalid element_order values.
Please add tests for invalid element_order values to ensure proper validation and error handling.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new integration tests in test_meshwell.py only run the CAD/mesh pipeline without validating the actual output; consider adding basic assertions on the number or types of generated prisms/surfaces to catch conversion issues early.
- The shared region_to_shapely_polygons utility now uses each_point_hole for hole extraction—ensure it correctly handles multiple or nested holes by adding targeted tests for those edge cases.
- In the main example of get_meshwell_3D, using component.name in file names assumes every component has a name attribute; consider adding a fallback or sanitizing logic to avoid runtime errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new integration tests in test_meshwell.py only run the CAD/mesh pipeline without validating the actual output; consider adding basic assertions on the number or types of generated prisms/surfaces to catch conversion issues early.
- The shared region_to_shapely_polygons utility now uses each_point_hole for hole extraction—ensure it correctly handles multiple or nested holes by adding targeted tests for those edge cases.
- In the __main__ example of get_meshwell_3D, using component.name in file names assumes every component has a name attribute; consider adding a fallback or sanitizing logic to avoid runtime errors.
## Individual Comments
### Comment 1
<location> `gplugins/meshwell/get_meshwell_3D.py:123-130` </location>
<code_context>
+ name_by="layer",
+ )
+
+ cad(entities_list=prisms, output_file=f"meshwell_prisms_3D_{c.name}.xao")
+ mesh(
+ input_file=f"meshwell_prisms_3D_{c.name}.xao",
+ output_file=f"meshwell_prisms_3D_{c.name}.msh",
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Output file naming may cause collisions if component names are not unique.
Overwriting may occur if component names are duplicated. Add unique identifiers to filenames or implement collision checks.
</issue_to_address>
### Comment 2
<location> `gplugins/meshwell/tests/test_meshwell.py:16-17` </location>
<code_context>
+)
+from shapely.geometry import LineString
+
+@pytest.mark.parametrize("component", [(bend_circular), (add_frame)])
+def test_prisms(component) -> None:
+ prisms = get_meshwell_prisms(
+ component=component(),
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding assertions to verify output files are created and non-empty.
Adding these assertions will help confirm that the mesh generation step completes successfully and does not fail silently.
</issue_to_address>
### Comment 3
<location> `gplugins/common/utils/geometry.py:10-14` </location>
<code_context>
def region_to_shapely_polygons(region: kf.kdb.Region) -> MultiPolygon:
"""Convert a kfactory Region to a list of Shapely polygons."""
polygons = []
for polygon_kdb in region.each():
# Extract exterior coordinates
exterior_coords = []
for point in polygon_kdb.each_point_hull():
exterior_coords.append((gf.kcl.to_um(point.x), gf.kcl.to_um(point.y)))
# Extract hole coordinates
holes = []
num_holes = polygon_kdb.holes()
for hole_idx in range(num_holes):
hole_coords = []
for point in polygon_kdb.each_point_hole(hole_idx):
hole_coords.append((gf.kcl.to_um(point.x), gf.kcl.to_um(point.y)))
holes.append(hole_coords)
# Create Shapely polygon
if holes:
polygon = Polygon(exterior_coords, holes)
else:
polygon = Polygon(exterior_coords)
polygons.append(polygon)
return MultiPolygon(polygons)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Convert for loop into list comprehension ([`list-comprehension`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/list-comprehension/))
```suggestion
exterior_coords = [
(gf.kcl.to_um(point.x), gf.kcl.to_um(point.y))
for point in polygon_kdb.each_point_hull()
]
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+123
to
+130
| cad(entities_list=prisms, output_file=f"meshwell_prisms_3D_{c.name}.xao") | ||
| mesh( | ||
| input_file=f"meshwell_prisms_3D_{c.name}.xao", | ||
| output_file=f"meshwell_prisms_3D_{c.name}.msh", | ||
| default_characteristic_length=1000, | ||
| dim=3, | ||
| verbosity=10, | ||
| ) |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Output file naming may cause collisions if component names are not unique.
Overwriting may occur if component names are duplicated. Add unique identifiers to filenames or implement collision checks.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Centralize geometry conversion by extracting region_to_shapely_polygons into a shared utility, update meshwell modules to use the new function, enhance demo scripts, enforce input validation in Palace simulations, and augment tests accordingly.
Enhancements:
Tests: