Fix meep #587
Merged
Fix meep #587
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
Reviewer's GuideThis PR refactors Meep geometry extraction to support layer exclusion and robust layer handling, optimizes simulation setup by flattening on a component copy, updates the example notebook with margin parameters and kernel metadata, and modernizes the adjoint optimizer’s algorithm parameter to use string identifiers. Sequence Diagram: Updated Meep Geometry Extraction with Layer ExclusionsequenceDiagram
participant Caller
participant GMFC as "get_meep_geometry_from_component()"
participant Component
participant LayerStack
Caller->>+GMFC: Call(component, layer_stack, exclude_layers, ...)
GMFC->>Component: component.get_polygons_points(merge=True)
Component-->>GMFC: polygons_per_layer
GMFC->>LayerStack: Access layer_stack.layers.values()
LayerStack-->>GMFC: levels (iterator/list)
loop For each 'level' in levels
GMFC->>GMFC: current_layer_tuple = Determine from level.layer / level.derived_layer.layer
GMFC->>GMFC: current_layer_index = get_layer(current_layer_tuple)
alt layer_index in exclude_layers OR layer_index not in polygons_per_layer
GMFC->>GMFC: Skip layer processing (continue loop)
else Process this layer
GMFC->>GMFC: zmin = level.zmin
GMFC->>GMFC: height = level.thickness
GMFC->>GMFC: polygons = polygons_per_layer[current_layer_index]
GMFC->>LayerStack: Get material_name for level.layer
LayerStack-->>GMFC: material_name
loop For each polygon in polygons
GMFC->>GMFC: Create Meep vertices (using zmin)
GMFC->>GMFC: Create mp.GeometricObject
GMFC->>GMFC: Add to results list
end
end
end
GMFC-->>-Caller: List of mp.GeometricObject
Class Diagram: Modified Function Signatures in gplugins.gmeep ModulesclassDiagram
class GetMeepGeometryModule {
<<gplugins.gmeep.get_meep_geometry>>
+get_meep_geometry_from_component(component: any, wavelength: float, is_3d: bool, dispersive: bool, exclude_layers: LayerSpecs | None, **kwargs) list~mp.GeometricObject~
}
class MeepAdjointOptimizationModule {
<<gplugins.gmeep.meep_adjoint_optimization>>
+run_meep_adjoint_optimizer(..., algorithm: str, ...) any
}
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}
There was a problem hiding this comment.
Hey @joamatab - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| wavelength: float = 1.55, | ||
| is_3d: bool = False, | ||
| dispersive: bool = False, | ||
| exclude_layers: LayerSpecs | None = None, |
There was a problem hiding this comment.
suggestion: Clarify exclude_layers type and usage
Consider renaming the parameter to exclude_layer_indices: Sequence[int] or converting it to set[int] for clearer intent and improved performance.
Suggested implementation:
exclude_layer_indices: Sequence[int] | None = None,) -> list[mp.GeometricObject]:
"""Returns Meep geometry from a gdsfactory component.
Args:
exclude_layer_indices: Sequence of layer indices to exclude from geometry generation.
kwargs: settings.
"""
component = gf.get_component(component=component, **kwargs)
polygons_per_layer = component.get_polygons_points(merge=True) layer_stack = layer_stack or get_layer_stack()
layer_to_thickness = layer_stack.get_layer_to_thickness()
# Convert exclude_layer_indices to set for fast lookup, if provided
exclude_layer_indices_set = set(exclude_layer_indices) if exclude_layer_indices is not None else set()- You will need to update all logic in the function that previously checked for excluded layers using
exclude_layersto now useexclude_layer_indices_set. For example, if you have code likeif layer in exclude_layers:, change it toif layer in exclude_layer_indices_set:. - Make sure to update any function calls or references to the old parameter name in the rest of the file and in any calling code.
- If the function is part of a public API, update any relevant documentation or type hints elsewhere in the codebase.
|
|
||
| geometry = [] | ||
| exclude_layers = exclude_layers or [] | ||
| layer_to_polygons = component_with_booleans.get_polygons_points() |
There was a problem hiding this comment.
suggestion: Remove unused layer_to_polygons assignment
Since layer_to_polygons is not used after introducing polygons_per_layer, removing it will make the code cleaner.
Comment on lines
84
to
87
| for polygon in polygons: | ||
| p = shapely.geometry.Polygon(polygon) | ||
| vertices = [mp.Vector3(p[0], p[1], zmin_um) for p in polygon] | ||
| material_name = layer_to_material[layer] |
There was a problem hiding this comment.
suggestion: Unnecessary shapely Polygon construction
Since p is not used, consider removing its instantiation to avoid unnecessary computation.
Suggested change
| for polygon in polygons: | |
| p = shapely.geometry.Polygon(polygon) | |
| vertices = [mp.Vector3(p[0], p[1], zmin_um) for p in polygon] | |
| material_name = layer_to_material[layer] | |
| for polygon in polygons: | |
| vertices = [mp.Vector3(p[0], p[1], zmin_um) for p in polygon] | |
| material_name = layer_to_material[layer] |
|
|
||
| layer_index = int(get_layer(layer_tuple)) | ||
|
|
||
| if layer_index in exclude_layers: |
There was a problem hiding this comment.
suggestion (performance): Optimize exclude_layers membership checks
Consider converting exclude_layers to a set before the loop to improve membership test performance for large collections.
Suggested implementation:
layer_index = int(get_layer(layer_tuple))
# Convert exclude_layers to a set for faster membership checks
if not isinstance(exclude_layers, set):
exclude_layers_set = set(exclude_layers) if exclude_layers is not None else set()
else:
exclude_layers_set = exclude_layers
if layer_index in exclude_layers_set:
continueYou should ensure that the initialization of exclude_layers_set happens outside the loop that contains this code, so the conversion to a set is not repeated on every iteration. If this code is inside a loop, move the conversion to just before the loop starts, and use exclude_layers_set in the loop body.
| update_variable: np.ndarray, | ||
| maximize_cost_function: bool = True, | ||
| algorithm: int = nlopt.LD_MMA, | ||
| algorithm: str = "LD_MMA", |
There was a problem hiding this comment.
suggestion: Rename algorithm parameter to reflect new string usage
Consider renaming the parameter to algorithm_name to clarify that it now expects a string, not an algorithm object.
Suggested implementation:
algorithm_name: str = "LD_MMA", algorithm = getattr(nlopt, algorithm_name)
solver = nlopt.opt(algorithm, number_of_params)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes meep plugin
fixes #575
@Arthaj-Octopus
@vvahidd
@threaddy
@joelslaby
@jan-david-fischbach
Summary by Sourcery
Improve the Meep plugin by adding layer exclusion and dynamic algorithm selection, streamlining geometry processing and simulation setup, and updating example notebooks.
New Features:
Enhancements:
Documentation: