Fix Palace capacitance computation for new meshwell#641
Merged
nikosavola merged 8 commits intofix_meshwellfrom Sep 18, 2025
Merged
Fix Palace capacitance computation for new meshwell#641nikosavola merged 8 commits intofix_meshwellfrom
nikosavola merged 8 commits intofix_meshwellfrom
Conversation
Contributor
Reviewer's GuideThis PR refactors the palace-based capacitance workflow to use the new meshwell meshing pipeline, removes extensive debug prints in favor of structured logging, simplifies JSON boundary assignments, and tightens layer and surface handling logic. Sequence diagram for Palace capacitance simulation workflow (Meshwell pipeline)sequenceDiagram
participant User
participant PalaceWorkflow
participant MeshwellCAD
participant MeshwellMesh
participant GMSH
participant Palace
participant Results
User->>PalaceWorkflow: run_capacitive_simulation_palace(component)
PalaceWorkflow->>get_component_with_net_layers: Prepare component layers
PalaceWorkflow->>MeshwellCAD: Generate CAD (.xao)
MeshwellCAD->>MeshwellMesh: Generate mesh (.msh)
MeshwellMesh->>GMSH: Read mesh file
GMSH->>Palace: Run simulation
Palace->>Results: Output capacitance matrix
Results->>User: Return ElectrostaticResults
Class diagram for get_component_with_net_layers refactorclassDiagram
class get_component_with_net_layers {
+component
+layer_stack
+port_names
+delimiter
+new_layers_init
+add_to_layerstack
+returns: net_component
}
class LayerLevel {
+layer
+name
+mesh_order
}
get_component_with_net_layers --> LayerLevel : creates new LayerLevels
LayerLevel <|-- DerivedLayer
LayerLevel <|-- LogicalLayer
Class diagram for simplified boundary assignment in Palace JSON generationclassDiagram
class PalaceJSON {
+Model.Mesh
+Domains.Materials
+Boundaries.Ground
+Boundaries.Terminal
+Solver.Order
+Solver.Electrostatic.Save
}
PalaceJSON --> Boundaries
class Boundaries {
+Ground.Attributes
+Terminal.Index
+Terminal.Attributes
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
nikosavola
added
dependencies
![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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `gplugins/palace/get_capacitance.py:169` </location>
<code_context>
- print(f" JSON config file: {json_file}")
- print(f" Simulation folder: {simulation_folder}")
- print(f" Working directory contents before Palace:")
- for item in simulation_folder.iterdir():
- print(f" - {item.name}")
-
</code_context>
<issue_to_address>
**suggestion:** Consider replacing print statements with logger calls for consistency.
Directory listing output is still using print; switching these to logger calls will ensure consistent logging throughout.
Suggested implementation:
```python
import logging
from kfactory import kdb
import gdsfactory as gf
import gmsh
from gdsfactory.generic_tech import LAYER_STACK
execute_and_stream_output,
run_async_with_event_loop,
)
from gplugins.common.utils.get_component_with_net_layers import (
get_component_with_net_layers,
logger = logging.getLogger(__name__)
```
```python
logger.info(f" JSON config file: {json_file}")
logger.info(f" Simulation folder: {simulation_folder}")
logger.info(f" Working directory contents before Palace:")
for item in simulation_folder.iterdir():
logger.info(f" - {item.name}")
```
</issue_to_address>
### Comment 2
<location> `gplugins/palace/get_capacitance.py:350-351` </location>
<code_context>
+ # Group signal BCs by ports
+ # TODO we need to remove the port-boundary surfaces for palace to work, why?
+ # TODO might as well remove the vacuum boundary and have just 2D sheets
+ metal_signal_surfaces_grouped = [
+ [e for e in metal_surfaces if port.name in e and boundary_delimiter not in e] for port in component.ports
]
-
</code_context>
<issue_to_address>
**issue:** The logic for grouping signal surfaces by port may not handle ambiguous or overlapping surface names robustly.
Using 'port.name in surface name' may cause incorrect groupings if names overlap or are ambiguous. Recommend stricter matching or an explicit mapping to prevent misassignment.
</issue_to_address>
### Comment 3
<location> `gplugins/palace/get_capacitance.py:375` </location>
<code_context>
for dimtag in gmsh.model.getPhysicalGroups()
}
+ # Use msh version 2.2 for MFEM / Palace compatibility, see https://mfem.org/mesh-formats/#gmsh-mesh-formats
+ gmsh.option.setNumber("Mesh.MshFileVersion", 2.2)
+ gmsh.write(str(simulation_folder / filename))
gmsh.finalize()
</code_context>
<issue_to_address>
**suggestion:** Setting the mesh file version to 2.2 is hardcoded; consider making this configurable.
Exposing the mesh file version as a parameter or adding documentation would improve future maintainability and adaptability.
Suggested implementation:
```python
# Set mesh file version for MFEM / Palace compatibility, see https://mfem.org/mesh-formats/#gmsh-mesh-formats
gmsh.option.setNumber("Mesh.MshFileVersion", mesh_file_version)
gmsh.write(str(simulation_folder / filename))
gmsh.finalize()
```
```python
def some_mesh_function(..., mesh_file_version: float = 2.2):
"""
...
Parameters
----------
...
mesh_file_version : float, optional
Version of the mesh file format to use (default is 2.2 for MFEM/Palace compatibility).
"""
```
You will need to:
1. Add `mesh_file_version` as a parameter to the function that contains the mesh generation code (replace `some_mesh_function` with the actual function name).
2. Pass `mesh_file_version` from wherever this function is called, or rely on the default.
3. Update any relevant documentation or usage examples to mention the new parameter.
</issue_to_address>
### Comment 4
<location> `gplugins/common/utils/get_component_with_net_layers.py:63` </location>
<code_context>
def get_component_with_net_layers(
component: Component,
layer_stack: LayerStack,
port_names: list[str],
delimiter: str = "#",
new_layers_init: tuple[int, int] = (10010, 0),
add_to_layerstack: bool = True,
) -> Component:
"""Returns a component where polygons touching a port are put on new logical layers.
Uses port's layer attribute to decide which polygons need to be renamed.
New layers are named "layername{delimiter}portname".
Args:
component: to process.
layer_stack: to process.
port_names: list of port_names to process into new layers.
delimiter: the new layer created is called "layername{delimiter}portname".
new_layers_init: initial layer number for the temporary new layers.
add_to_layerstack: True by default, but can be set to False to disable parsing of the layerstack.
"""
# Initialize returned component
net_component = component.copy()
new_layerlevels = list()
# For each port to consider, convert relevant polygons
for i, port_name in enumerate(port_names):
port = component.ports[port_name]
# Get original port layer polygons, and modify a new component without that layer
polygons = (
net_component.extract(layers=(port.layer,))
.get_polygons()
.get(port.layer, [])
)
net_component = net_component.remove_layers(layers=(port.layer,))
for polygon in polygons:
# If polygon belongs to port, create a unique new layer, and add the polygon to it
if polygon.sized(int(3 * gf.kcl.dbu)).inside(
kdb.Point(*port.to_itype().center)
):
try:
derived_layerlevels_touching_port = [
e
for e in layer_stack.layers.values()
if e.derived_layer is not None
and e not in new_layerlevels
and (
e.derived_layer.layer.layer,
e.derived_layer.layer.datatype,
)
== (
port.layer_info.layer,
port.layer_info.datatype,
)
]
logical_layerlevels_touching_port = [
e
for e in layer_stack.layers.values()
if not isinstance(
e.layer, gf.technology.layer_stack.DerivedLayer
)
and e not in new_layerlevels
and (e.layer.layer.layer, e.layer.layer.datatype)
== (port.layer_info.layer, port.layer_info.datatype)
]
layerlevels_touching_port = (
derived_layerlevels_touching_port
+ logical_layerlevels_touching_port
)
except KeyError as e:
raise KeyError(
"Make sure your `layer_stack` contains all layers with ports"
) from e
for j, old_layerlevel in enumerate(layerlevels_touching_port):
new_layer_number = (
new_layers_init[0] + i,
new_layers_init[1] + j,
)
if add_to_layerstack:
# new_layer = copy.deepcopy(layer_stack.layers[old_layerlevel])
new_layerlevel = copy.deepcopy(old_layerlevel)
new_layerlevel.layer = LogicalLayer(
layer=(
new_layers_init[0] + i,
new_layers_init[1] + j,
)
)
new_layerlevel.name = (
f"{old_layerlevel.name}{delimiter}{port_name}"
)
# Increase mesh order to ensure new layer is on top old
new_layerlevel.mesh_order = old_layerlevel.mesh_order - 1
layer_stack.layers[
f"{old_layerlevel.name}{delimiter}{port_name}"
] = new_layerlevel
new_layerlevels.append(new_layerlevel)
net_component.add_polygon(polygon, layer=new_layer_number)
# Otherwise put the polygon back on the same layer
else:
net_component.add_polygon(polygon, layer=port.layer)
net_component.name = f"{component.name}_net_layers"
return net_component
</code_context>
<issue_to_address>
**suggestion (code-quality):** We've found these issues:
- Replace `list()` with `[]` ([`list-literal`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/list-literal/))
- Low code quality found in get\_component\_with\_net\_layers - 19% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
```suggestion
new_layerlevels = []
```
<br/><details><summary>Explanation</summary>The most concise and Pythonic way to create a list is to use the `[]` notation.
This fits in with the way we create lists with elements, saving a bit of mental
energy that might be taken up with thinking about two different ways of creating
lists.
```python
x = ["first", "second"]
```
Doing things this way has the added advantage of being a nice little performance
improvement.
Here are the timings before and after the change:
```
$ python3 -m timeit "x = list()"
5000000 loops, best of 5: 63.3 nsec per loop
```
```
$ python3 -m timeit "x = []"
20000000 loops, best of 5: 15.8 nsec per loop
```
Similar reasoning and performance results hold for replacing `dict()` with `{}`.
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines.
- Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.</details>
</issue_to_address>
### Comment 5
<location> `gplugins/palace/get_capacitance.py:144` </location>
<code_context>
def _palace(simulation_folder: Path, name: str, n_processes: int = 1) -> None:
"""Run simulations with Palace."""
# Try to find palace in PATH first
palace = shutil.which("palace")
# If not found, try to load it via Spack
if palace is None:
raise RuntimeError(
"palace not found. Make sure it is available in your PATH or via Spack."
)
else:
# Palace found in PATH, use the original method
json_file = simulation_folder / f"{Path(name).stem}.json"
logger.info(f" Running Palace simulation...")
logger.info(f" Palace executable: {palace}")
logger.info(f" JSON config file: {json_file}")
logger.info(f" Simulation folder: {simulation_folder}")
logger.info(f" Working directory contents before Palace:")
for item in simulation_folder.iterdir():
print(f" - {item.name}")
try:
run_async_with_event_loop(
execute_and_stream_output(
[palace, json_file]
if n_processes == 1
else [palace, "-np", str(n_processes), json_file],
shell=False,
log_file_dir=simulation_folder,
log_file_str=json_file.stem + "_palace",
cwd=simulation_folder,
)
)
except Exception as e:
logger.info(f" ❌ Palace execution failed: {e}")
raise
# Check results after Palace execution (regardless of method used)
logger.info(f" Palace execution completed!")
logger.debug(f" Working directory contents after Palace:")
for item in simulation_folder.iterdir():
logger.debug(f" - {item.name}")
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Extract code out into function ([`extract-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-method/))
- Replace f-string with no interpolated values with string [×4] ([`remove-redundant-fstring`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-redundant-fstring/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
joamatab
approved these changes
Sep 18, 2025
Contributor
joamatab
left a comment
joamatab
left a comment
There was a problem hiding this comment.
some tests seem to fail
Member
Author
The femwell tests fail because they fail in the main meshwell PR as well. Palace is failing because this doesn't have changes from #643 yet. |
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
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
Replace GMSH-based meshing with Meshwell’s 3D pipeline, streamline boundary assignment logic, and clean up debug flow in Palace capacitance simulation
New Features:
Enhancements:
Chores: