Feature: VLSIR-based Netlisting Support #51
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
…onents, and added/passed new tests
Contributor
Reviewer's GuideAdds VLSIR-based netlisting support for the GF180 PDK by introducing a GDSFactory-to-VLSIR conversion utility, annotating key device cells with vlsir metadata suitable for nested sub-model spice libraries, adding tests and regression data for these annotations, and wiring in vlsir/vlsirtools dependencies and basic simulator model files. Sequence diagram for converting a component to a SPICE netlist with VLSIRsequenceDiagram
actor User
participant Component
participant ToVlsir as to_vlsir_module
participant VlsirPackage
participant Vlsirtools as vlsirtools_netlist
User->>Component: configure_with_vlsir_metadata
User->>ToVlsir: to_spice(component, domain, fmt)
ToVlsir->>ToVlsir: validate_vlsir_metadata(component)
ToVlsir->>VlsirPackage: to_proto(component, domain)
VlsirPackage-->>ToVlsir: package_instance
ToVlsir->>Vlsirtools: netlist(package_instance, dest, fmt)
Vlsirtools-->>ToVlsir: spice_netlist_string
ToVlsir-->>User: spice_netlist_string
Class diagram for the new GDSFactory to VLSIR conversion utilitiesclassDiagram
class to_vlsir_module {
<<module>>
+REQUIRED_VLSIR_FIELDS
+_SPICE_TYPE_MAP
+validate_vlsir_metadata(component)
+_spice_type_to_proto(spice_type)
+to_proto(component, domain)
+to_spice(component, domain, fmt)
}
class Component {
+name
+ports
+info
}
class VlsirPackage {
+domain
+ext_modules
+modules
}
class ExternalModule {
+name
+spicetype
+signals
+ports
}
class Module {
+name
+signals
+ports
+instances
}
class Instance {
+name
+module
+connections
+parameters
}
class VlsirtoolsNetlist {
+netlist(pkg, dest, fmt)
}
Component --> to_vlsir_module : passes_to
to_vlsir_module ..> VlsirPackage : creates
to_vlsir_module ..> ExternalModule : creates
to_vlsir_module ..> Module : creates
to_vlsir_module ..> Instance : creates
to_vlsir_module ..> VlsirtoolsNetlist : uses_in_to_spice
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 - I've found 5 issues, and left some high level feedback:
- The
to_protoimplementation ignores theport_mapmetadata and always usesport_orderdirectly, so components that rely on a mapping between GDSF port names and SPICE/VLSIR names will not netlist as intended; consider usingport_mapwhen wiring instance connections and/or top-level signals. validate_vlsir_metadataonly checks thatport_mapkeys exist as component ports but does not verify that the mapped values are a subset ofport_order, which can allow inconsistent metadata; adding a check thatset(port_map.values()) ⊆ set(port_order)would catch these cases early.- Both
hv_genandbulk_gr_gennow unconditionally raiseNotImplementedError, which makes these helpers unusable at runtime; if they are intended to be deprecated, consider removing them or gating the exception so existing call sites do not break unexpectedly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `to_proto` implementation ignores the `port_map` metadata and always uses `port_order` directly, so components that rely on a mapping between GDSF port names and SPICE/VLSIR names will not netlist as intended; consider using `port_map` when wiring instance connections and/or top-level signals.
- `validate_vlsir_metadata` only checks that `port_map` keys exist as component ports but does not verify that the mapped values are a subset of `port_order`, which can allow inconsistent metadata; adding a check that `set(port_map.values()) ⊆ set(port_order)` would catch these cases early.
- Both `hv_gen` and `bulk_gr_gen` now unconditionally raise `NotImplementedError`, which makes these helpers unusable at runtime; if they are intended to be deprecated, consider removing them or gating the exception so existing call sites do not break unexpectedly.
## Individual Comments
### Comment 1
<location> `gf180mcu/cells/fet.py:598` </location>
<code_context>
v5x.ymin = dg.ymin
+ # TODO: Implement/delete
+ raise NotImplementedError()
# return c
</code_context>
<issue_to_address>
**issue (bug_risk):** Unconditional NotImplementedError in hv_gen will break any existing callers.
This will cause any code calling `hv_gen` to fail at runtime, which is a breaking change if it’s part of the public PDK API or used by other cells. If you’re deprecating it, either remove all usages in this PR or guard the exception behind a flag/config so existing designs continue to run.
</issue_to_address>
### Comment 2
<location> `gf180mcu/models/to_vlsir.py:119-128` </location>
<code_context>
+ port_map = vlsir_info.get("port_map")
</code_context>
<issue_to_address>
**issue (bug_risk):** port_map is validated but never used when building the VLSIR package, which can silently miswire ports.
`validate_vlsir_metadata` implies `port_map` should map component port names to VLSIR/SPICE names, but `to_proto` ignores it and relies solely on `port_order` for both the external module and top-level signals. When component ports differ from VLSIR names (e.g., `{"r0": "1"}`), validation still passes but the mapping is never applied, producing silently miswired netlists. Either wire top-level signals through `port_map`, or reject configs that specify `port_map` until it is correctly supported.
</issue_to_address>
### Comment 3
<location> `gf180mcu/cells/cap_mos.py:8` </location>
<code_context>
from gf180mcu.cells.via_generator import via_generator, via_stack
from gf180mcu.layers import layer
+# TODO: For SPICE modelling, we must deduced
+
</code_context>
<issue_to_address>
**nitpick (typo):** Incomplete TODO text is unclear and slightly confusing.
The phrase `we must deduced` seems truncated and ungrammatical, which obscures the intent of the TODO. Please rephrase to clearly state what must be deduced for SPICE modelling (e.g., model name, biasing, or parameter extraction).
```suggestion
# TODO: For SPICE modelling, we must deduce the appropriate SPICE model name
# and parameter set for this capacitor structure (e.g., geometry- and
# bias-dependent parameters), and associate them with this cell.
```
</issue_to_address>
### Comment 4
<location> `tests/test_components.py:12-14` </location>
<code_context>
+from gf180mcu.models.to_vlsir import to_proto, to_spice, validate_vlsir_metadata
-skip_test = {"res_dev"}
+skip_test = {"res_dev", "nfet", "pfet"}
cells = PDK.cells
cell_names = set(cells.keys()) - set(skip_test)
dirpath = pathlib.Path(__file__).absolute().parent / "gds_ref"
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding dedicated VLSIR tests for `nfet`/`pfet` instead of skipping them from the parametrized suite
Since `nfet`/`pfet` now have non-trivial VLSIR metadata (voltage-based model selection, `spice_type`, empty `port_map`, etc.), it would be good to add a couple of focused tests that:
- Instantiate them at different `volt` values and assert the expected `model`/`spice_lib` in `component.info["vlsir"`].
- Exercise `validate_vlsir_metadata`, `to_proto`, and `to_spice` to confirm the metadata is self-consistent.
If including them in the parametrized suite is too expensive (runtime/deps), small dedicated tests would restore coverage with minimal overhead.
Suggested implementation:
```python
import pathlib
import gdsfactory as gf
```
```python
from gf180mcu.models.to_vlsir import to_proto, to_spice, validate_vlsir_metadata
skip_test = {"res_dev", "nfet", "pfet"}
cells = PDK.cells
cell_names = set(cells.keys()) - set(skip_test)
dirpath = pathlib.Path(__file__).absolute().parent / "gds_ref"
```
```python
@pytest.mark.parametrize("component_name", cell_names)
def test_vlsir_to_proto(component_name: str) -> None:
"""Test to_proto and metadata validation for cells with vlsir metadata."""
component = cells[component_name]()
vlsir_metadata = component.info.get("vlsir")
if vlsir_metadata is None:
pytest.skip(f"{component_name} has no VLSIR metadata")
# Ensure VLSIR metadata is self-consistent
validate_vlsir_metadata(vlsir_metadata)
# Ensure we can export to VLSIR proto without error
proto = to_proto(component)
assert proto is not None
# Ensure we can export to SPICE without error
spice_str = to_spice(component)
assert isinstance(spice_str, str)
assert spice_str.strip()
```
```python
from gf180mcu.models.to_vlsir import to_proto, to_spice, validate_vlsir_metadata
skip_test = {"res_dev", "nfet", "pfet"}
cells = PDK.cells
cell_names = set(cells.keys()) - set(skip_test)
dirpath = pathlib.Path(__file__).absolute().parent / "gds_ref"
) -> None:
"""Avoid regressions when exporting settings."""
data_regression.check(component.to_dict())
@pytest.mark.parametrize("component_name", cell_names)
def test_vlsir_to_proto(component_name: str) -> None:
"""Test to_proto and metadata validation for cells with vlsir metadata."""
component = cells[component_name]()
vlsir_metadata = component.info.get("vlsir")
if vlsir_metadata is None:
pytest.skip(f"{component_name} has no VLSIR metadata")
# Ensure VLSIR metadata is self-consistent
validate_vlsir_metadata(vlsir_metadata)
# Ensure we can export to VLSIR proto without error
proto = to_proto(component)
assert proto is not None
# Ensure we can export to SPICE without error
spice_str = to_spice(component)
assert isinstance(spice_str, str)
assert spice_str.strip()
@pytest.mark.parametrize("volt_low, volt_high", [(3.3, 5.0)])
@pytest.mark.parametrize("device_name", ["nfet", "pfet"])
def test_fet_vlsir_voltage_dependent_metadata(device_name: str, volt_low: float, volt_high: float) -> None:
"""Dedicated VLSIR tests for nfet/pfet with voltage-dependent metadata.
- Instantiate the device at different voltages.
- Verify that the VLSIR metadata (e.g., model / spice_lib) changes with voltage.
- Exercise validate_vlsir_metadata, to_proto, and to_spice.
"""
# Instantiate at two different voltages
fet_low = cells[device_name](volt=volt_low)
fet_high = cells[device_name](volt=volt_high)
vlsir_low = fet_low.info.get("vlsir")
vlsir_high = fet_high.info.get("vlsir")
assert vlsir_low is not None, f"{device_name} at {volt_low} V is missing VLSIR metadata"
assert vlsir_high is not None, f"{device_name} at {volt_high} V is missing VLSIR metadata"
# Validate both metadata variants
validate_vlsir_metadata(vlsir_low)
validate_vlsir_metadata(vlsir_high)
# Expect some voltage-dependent difference in model and/or spice_lib
model_low = vlsir_low.get("model")
model_high = vlsir_high.get("model")
spice_lib_low = vlsir_low.get("spice_lib")
spice_lib_high = vlsir_high.get("spice_lib")
assert (model_low is not None) or (spice_lib_low is not None), (
f"{device_name} VLSIR metadata should define at least a model or spice_lib"
)
assert (model_high is not None) or (spice_lib_high is not None), (
f"{device_name} VLSIR metadata should define at least a model or spice_lib"
)
# At least one of model / spice_lib should be voltage-dependent
metadata_differs = (model_low != model_high) or (spice_lib_low != spice_lib_high)
assert metadata_differs, (
f"{device_name} VLSIR metadata should vary with voltage: "
f"(model_low={model_low}, model_high={model_high}, "
f"spice_lib_low={spice_lib_low}, spice_lib_high={spice_lib_high})"
)
# Ensure we can export both variants to VLSIR proto and SPICE
proto_low = to_proto(fet_low)
proto_high = to_proto(fet_high)
assert proto_low is not None
assert proto_high is not None
spice_low = to_spice(fet_low)
spice_high = to_spice(fet_high)
assert isinstance(spice_low, str) and spice_low.strip()
assert isinstance(spice_high, str) and spice_high.strip()
```
1. If the actual allowed `volt` values for `nfet`/`pfet` differ from `3.3` and `5.0`, update the `@pytest.mark.parametrize("volt_low, volt_high", ...)` decorator to use the correct pair (or pairs) of voltages supported by the PDK.
2. If the VLSIR metadata schema for `nfet`/`pfet` uses different keys than `"model"` or `"spice_lib"` to represent the selected model/library, adjust the `model_low/model_high` and `spice_lib_low/spice_lib_high` extraction accordingly.
3. If creating `nfet`/`pfet` requires additional keyword arguments beyond `volt` (e.g., `l`, `w`, `m`), add them to the `cells[device_name](...)` calls in `test_fet_vlsir_voltage_dependent_metadata`.
</issue_to_address>
### Comment 5
<location> `tests/test_components.py:73-82` </location>
<code_context>
+class TestVlsirValidationErrors:
</code_context>
<issue_to_address>
**suggestion (testing):** Add at least one validation test using a real PDK cell’s `vlsir` metadata
The existing tests cover failure modes with synthetic components. To also confirm that real PDK wiring is correct, please add at least one success-path test that:
- Uses one or two representative PDK cells with `vlsir` metadata (e.g., a resistor and a diode),
- Calls `validate_vlsir_metadata` on them, and
- Asserts successful return of the expected normalized metadata.
This will help catch regressions from PDK metadata changes (e.g., port renames or `port_order` updates) earlier in the flow.
Suggested implementation:
```python
with pytest.raises(ValueError, match="missing required 'vlsir' metadata"):
validate_vlsir_metadata(c)
class TestVlsirValidationSuccess:
"""Test successful validation for real PDK cells with vlsir metadata."""
def test_real_pdk_resistor_and_diode_metadata(self) -> None:
"""Use representative PDK cells and assert successful normalized metadata."""
# Use representative passive devices; these should be backed by the active PDK.
resistor = gf.components.resistor()
diode = gf.components.diode()
for component in (resistor, diode):
# Only run this test when the cell exposes vlsir metadata in the active PDK.
if "vlsir" not in component.info:
pytest.skip(f"{component.name!r} does not have vlsir metadata in the active PDK")
normalized = validate_vlsir_metadata(component)
# Basic contract checks for the normalized metadata structure.
# Adjust these assertions if validate_vlsir_metadata exposes a richer schema.
assert isinstance(normalized, dict)
assert "ports" in normalized, "normalized metadata must expose a 'ports' mapping"
assert isinstance(normalized["ports"], dict)
# All normalized ports must correspond to actual component ports.
for port_name in normalized["ports"]:
assert port_name in component.ports
# The validator should be idempotent: validating normalized metadata again
# should return an equivalent structure.
normalized_again = validate_vlsir_metadata(component)
assert normalized_again == normalized
```
To make this compile and properly exercise real PDK cells, you will likely need to:
1. Ensure the following imports exist near the top of `tests/test_components.py` (add them if missing):
- `import gdsfactory as gf`
- `import pytest`
- `from gdsfactory.components.vlsir import validate_vlsir_metadata`
2. If `validate_vlsir_metadata` lives in a different module in your codebase, update the import path accordingly.
3. If your active PDK exposes different representative cells with `vlsir` metadata (e.g., specific gf180 resistor/diode names rather than `gf.components.resistor()` / `gf.components.diode()`), swap those constructors for the appropriate PDK-backed factories while keeping the same assertion pattern.
4. If the normalized metadata returned by `validate_vlsir_metadata` has a more specific schema (e.g., keys like `module_name`, `port_order`, etc.), tighten the assertions in `test_real_pdk_resistor_and_diode_metadata` to match that concrete structure.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| v5x.ymin = dg.ymin | ||
|
|
||
| # TODO: Implement/delete | ||
| raise NotImplementedError() |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Unconditional NotImplementedError in hv_gen will break any existing callers.
This will cause any code calling hv_gen to fail at runtime, which is a breaking change if it’s part of the public PDK API or used by other cells. If you’re deprecating it, either remove all usages in this PR or guard the exception behind a flag/config so existing designs continue to run.
| from gf180mcu.cells.via_generator import via_generator, via_stack | ||
| from gf180mcu.layers import layer | ||
|
|
||
| # TODO: For SPICE modelling, we must deduced |
Contributor
There was a problem hiding this comment.
nitpick (typo): Incomplete TODO text is unclear and slightly confusing.
The phrase we must deduced seems truncated and ungrammatical, which obscures the intent of the TODO. Please rephrase to clearly state what must be deduced for SPICE modelling (e.g., model name, biasing, or parameter extraction).
Suggested change
| # TODO: For SPICE modelling, we must deduced | |
| # TODO: For SPICE modelling, we must deduce the appropriate SPICE model name | |
| # and parameter set for this capacitor structure (e.g., geometry- and | |
| # bias-dependent parameters), and associate them with this cell. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Addresses Issue in #25 - much the same update as in gdsfactory/IHP#66 but with special considerations for the nested sub-model architecture favored by GF180MCU foundry PDK authors with giant monolibs for both open-source simulators attached.
Summary by Sourcery
Add VLSIR-backed netlisting support for key gf180mcu devices and integrate it into the PDK and tests.
New Features:
Enhancements:
Build:
Documentation:
Tests:
Chores: