feat: Simulation.run_local() for MEEP and sim fixes#144
Open
benvial wants to merge 25 commits into
Open
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a major refactor of the material system to support frequency-dependent dispersion and anisotropic tensors across both MEEP and Palace solvers, including a new PDK overlay system and support for shaped dielectric volumes. Key improvements involve the implementation of Sellmeier and Lorentzian dispersion models and a three-tier material resolution strategy. Critical feedback highlights that the MEEP material builder ignores new fields, identifies a unit mismatch in conductivity conversion for MEEP, and points out logic errors in tensor rotation. Furthermore, several debug artifacts were found in notebooks and scripts, and a redundant function call was identified in the Palace material resolution logic.
Comment on lines
61
to
70
| for name, props in config["materials"].items(): | ||
| n = props["refractive_index"] | ||
| k = props["extinction_coeff"] | ||
| if k > 0: | ||
| materials[name] = mp.Medium(index=n, D_conductivity=2 * cmath.pi * k / n) | ||
| eps_diag = props.get("epsilon_diag") | ||
| if eps_diag is not None: | ||
| if len(set(eps_diag)) == 1: | ||
| materials[name] = mp.Medium(epsilon=eps_diag[0]) | ||
| else: | ||
| materials[name] = mp.Medium(epsilon_diag=eps_diag) | ||
| else: | ||
| materials[name] = mp.Medium(index=n) | ||
| materials[name] = mp.Medium(epsilon=1.0) | ||
| return materials |
There was a problem hiding this comment.
The build_materials function has not been updated to handle the new fields in MaterialData. It currently ignores epsilon_offdiag, mu_diag, D_conductivity, D_conductivity_diag, and epsilon_susceptibilities. This means that dispersion models, loss, and anisotropy will be ignored when running MEEP simulations locally via run_local(). The function should be refactored to pass all available properties to the mp.Medium constructor.
| Returns: | ||
| Conductivity in S/m | ||
| """ | ||
| return 2.0 * math.pi * freq_hz * _EPS0_SI * permittivity * loss_tangent |
There was a problem hiding this comment.
The loss_tangent_to_conductivity function returns conductivity in SI units (S/m). However, MEEP's conductivity and D_conductivity parameters expect values in MEEP units (dimensionless, typically
Comment on lines
+208
to
+215
| """ | ||
| rotated = [[0.0] * 3 for _ in range(3)] | ||
| for i in range(3): | ||
| for j in range(3): | ||
| for k in range(3): | ||
| rotated[i][j] += material_axes[i][k] * diag[k] * material_axes[j][k] | ||
|
|
||
| return [rotated[0][1], rotated[0][2], rotated[1][2]] |
There was a problem hiding this comment.
The tensor rotation logic in _rotate_diagonal_tensor has two issues:
- The indices in the matrix multiplication are swapped. If
material_axesrows are the local axis vectors ($R$ ), the global tensor$T$ is$R^T D R$ , which corresponds to$T_{ij} = \sum_k R_{ki} D_{kk} R_{kj}$ . The current code computes$R D R^T$ . - Rotation generally affects both diagonal and off-diagonal components. This function should return the full rotated tensor so that
_resolved_to_material_datacan updatedata.epsilon_diagas well. Currently,data.epsilon_diagremains set to the local diagonal values whileepsilon_offdiagis set to global values, which is inconsistent.
References
- Ensure appropriate handling of anisotropic tensors and coordinate transformations.
| "print(sim.validate_config())" | ||
| "print(sim.validate_config())\n", | ||
| "\n", | ||
| "xsxs" |
There was a problem hiding this comment.
Debug artifact xsxs found in notebook cell. This will cause a NameError when the notebook is executed.
|
|
||
| print(sim.validate_config()) | ||
|
|
||
| xsxs |
There was a problem hiding this comment.
Debug artifact xsxs found. Please remove it before merging.
| "# Static PNG\n", | ||
| "sim.plot_mesh(show_groups=[\"metal\", \"P\"])" | ||
| "sim.plot_mesh(show_groups=[\"metal\", \"P\"])\n", | ||
| "xsx" |
There was a problem hiding this comment.
Debug artifact xsx found in notebook cell.
| resolved[name] = dict(props) | ||
| continue | ||
|
|
||
| evaluated = resolve_material_at_wavelength(name, wavelength_um) |
There was a problem hiding this comment.
Redundant call to resolve_material_at_wavelength. The evaluated variable is already assigned and checked at line 51.
Add dispersion model types (ValidityRange, SellmeierTerm, LorentzianTerm, DispersionModel) to the materials database. Each material entry can store multiple dispersion models covering different frequency regimes (e.g. SiO2: Sellmeier for optical, constant-ε for RF), each annotated with validity range and citation. Key changes: - MaterialProperties gains dispersion_models, conductivity_diagonal - evaluate_at_wavelength/frequency resolves the correct model - resolve_material_at_wavelength() with validity checking and warnings - should_enable_dispersion() auto-heuristic (Δn/n > 0.5%) - ResolvedMaterial carries evaluation metadata - MEEP materials.py uses wavelength-aware resolution - MaterialData config supports anisotropic + dispersive fields - LorentzianPoleConfig for susceptibility poles - FDTD.dispersion flag (auto/true/false) - Palace config_generator handles conductivity_diagonal - PDK overlay system (load_overlay, merge_overlay) - MATERIALS_DB updated with Sellmeier fits and validity ranges - Comprehensive tests for dispersion models and overlays
Conductors have no optical data; silently skip them instead of emitting spurious warnings. Add germanium (Ge) to MATERIALS_DB with Sellmeier fit from Barnes & Piltch 1979 (2.5-12 µm).
Replace Unicode symbols with ASCII equivalents for cp1252 compatibility. Add missing docstrings for interrogate coverage. Fix line length violations. Fix ruff lint warnings in tests.
Add overlay parameter to resolve_material_at_wavelength() and should_enable_dispersion(). Add _resolve_with_overlay() helper implementing priority: user override > PDK overlay > built-in DB. Thread overlay through MEEP resolve_materials(). Add Simulation.load_pdk_overlay() and pass overlay in build_config().
Map ResolvedMaterial anisotropic fields to MaterialData: epsilon_diag, mu_diag, D_conductivity, D_conductivity_diag, epsilon_offdiag. Add loss_tangent_to_conductivity() converter (sigma = 2*pi*f*eps0*er*tand). Add _rotate_diagonal_tensor() for material_axes rotation. Add _resolved_to_material_data() as unified converter.
Implement sellmeier_to_lorentzian_poles() converter mapping Sellmeier terms to MEEP LorentzianSusceptibility poles (w0=1/sqrt(C), sigma=B*w0^2, gamma=0). Add lorentzian_to_meep_poles() for direct Lorentzian mapping. Reimplement resolve_materials_with_dispersion() with full dispersion flag logic: auto evaluates dn/n per material, true forces all dispersive, false forces constant-epsilon. Wire through Simulation.build_config() when solver.dispersion != 'false'.
Add resolve_palace_materials_at_frequency() to evaluate dispersion models at a target frequency, producing a materials dict for Palace config generation. Add frequency_hz parameter to generate_palace_config() and write_config(). When provided, material permittivity is evaluated from Sellmeier/Lorentzian models at that frequency instead of using static values. Implements the RFC's external frequency loop strategy for Palace (which lacks native epsilon(f) support).
Merge permittivity_diagonal → permittivity (float | list[float]), conductivity_diagonal → conductivity, loss_tangent_diagonal → loss_tangent. Add _as_list() and _is_tensor() helpers. Add scalar convenience properties. Remove pydantic ge/le constraints from union-typed fields. Update overlays, MEEP, Palace, and all tests. 541 tests pass, all pre-commit hooks pass.
Replace refractive_index + extinction_coeff with permittivity + loss_tangent as the sole solver-facing representation. Removed from ResolvedMaterial, MaterialProperties, DispersionModel, MaterialData, and all downstream consumers (MEEP, Palace, overlays, ports, script). - evaluate_at_wavelength() always sets permittivity directly - index_variation() computes deps/eps instead of dn/n - MaterialData uses epsilon_diag as primary field (no refractive_index) - build_materials() in script.py uses mp.Medium(epsilon=) directly - Removed MaterialProperties.optical() classmethod - Removed DispersionModel.refractive_index field - 540 tests pass, all 23 pre-commit hooks pass
…loat shorthand
Replace refractive-index-based Material(n, k) with permittivity-based
Material(permittivity, loss_tangent). Float shorthand like {"si": 12.0}
is interpreted as permittivity. Remove float-to-n**2 bridge in
_material_overrides. Update dispersion docstrings to deps/eps. Fix
pre-existing indentation bug in _cloud_fixtures.py.
…ter freq Add f parameter to set_driven(): sim.set_driven(f=50e9) sets fmin=fmax with num_points=1. Explicit fmin/fmax override f. Add DrivenConfig.center_frequency property. Remove frequency_hz from generate_palace_config/write_config; dispersion is now auto-evaluated at (fmin+fmax)/2. Fix c: 3e8 -> 299_792_458 everywhere. Remove stray 'xsx' in palace_microstrip notebook.
Auto-detect shaped dielectrics from layer_type=dielectric + thin patterned layers (<5um) with polygon geometry. Remove manual shaped_dielectrics parameter and layer.shaped field entirely. Photonic stacks (ubcpdk) get BOX preserved as SiO2 via has_patterned_dielectrics auto-detection. Silicon conductivity dropped when Sellmeier covers target frequency.
…e check Shaped dielectric detection now uses _detect_shaped_dielectric_layers(): a dielectric layer is shaped iff it has polygon geometry AND its (material, z-range) is NOT already covered by a stack.dielectrics box. Thickness threshold was fragile; box-coverage is structurally correct.
Silicon: Salzberg & Villa 3-term with correct B3=1.541/C3=1104^2 (n=3.478 at 1550nm, was 3.634 with wrong 3rd term). SiO2: Malitson 1965 full-precision coefficients, wider 0.21-6.7um range. Si3N4: Luke 2015 NIR 2-term model (meep: Si3N4_NIR), wider 0.31-5.5um. Sapphire: Malitson & Dodge 1972 3-term ordinary-axis (n_o=1.746 at 1550nm). Germanium: Icenogle 1979 with epsilon_inf=9.28 (meep: Ge).
…equency-aware behavior Material.type (conductor/dielectric/semiconductor) is frequency-independent but material behavior is not — Si is dielectric at optical, conductive at RF. Replace with ResolvedMaterial.behavior property that decides "conductive" vs "dielectric" at evaluation time using conductivity threshold (>=1e4 S/m) and dispersive model presence. BREAKING CHANGE: MaterialProperties.type field removed. Use resolved.behavior instead of material_is_conductor/dielectric.
…erials Conductive materials (Al, Cu, etc.) have no permittivity field — this is expected, not an error. Move behavior check before permittivity-is-None check in resolve_materials_with_dispersion, and skip warning in resolve_material_at_wavelength when behavior is conductive.
SiO2 validity range updated 3.71->6.7um, source string relaxed. Ge dispersion test moved to wl=3.0/bw=1.0 where variation exceeds threshold (new Icenogle model is much flatter at 4um than old model).
MEEP run_local() writes config/GDS/script, then executes run_meep.py locally via Python (with meep installed). Supports MPI via num_processes parameter. Returns SParameterResult parsed from CSV. Also updates silicon n@1550nm test bounds from 3.5-4.2 to 3.4-3.6 after correcting Sellmeier coefficients (n=3.478, was 4.034).
benvial
force-pushed
the
feat/meep-run-local-and-sim-fixes
branch
2 times, most recently
from
5f028d4 to
04b2237
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #144 +/- ##
==========================================
+ Coverage 55.55% 57.42% +1.87%
==========================================
Files 59 61 +2
Lines 6966 7655 +689
Branches 1270 1445 +175
==========================================
+ Hits 3870 4396 +526
- Misses 2697 2827 +130
- Partials 399 432 +33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Previously run_local() required manual write_config() call before running, while run() auto-generated config.json. Now both methods auto-generate config.json when missing.
Stale config from prior runs with different settings (e.g. include_substrate toggled) caused Palace 'Material attribute tags must correspond to attributes in the mesh' errors. Palace run_local() was the only run* method that skipped config regeneration; Meep and Palace cloud run() always overwrote it.
benvial
force-pushed
the
feat/meep-run-local-and-sim-fixes
branch
from
04b2237 to
634208c
Compare
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
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.
Summary
Simulation.run_local()for MEEP simulationsrun_local()to auto-callwrite_config()matchingrun()behaviorscipy.constants.cinstead of hardcoded 299792458run_local()in meep straight example notebookDependencies
Depends on PR #143 (frequency-dependent materials) — uses the refactored material API.