feat(mesh): decimate_tolerance, verbosity, and stale tag fix#145
Open
benvial wants to merge 24 commits into
Open
feat(mesh): decimate_tolerance, verbosity, and stale tag fix#145benvial wants to merge 24 commits into
benvial wants to merge 24 commits into
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 comprehensive material dispersion system for both MEEP and Palace simulations, refactoring the MaterialProperties model to support Sellmeier and Lorentzian models, unified tensor fields, and frequency-aware resolution. It also adds a PDK overlay system for foundry-specific material data and implements support for shaped dielectric volumes in the Palace meshing pipeline. Feedback highlights leftover debugging code in notebooks, a TypeError in the overlay loading logic, and a potential ZeroDivisionError in Sellmeier calculations. Additionally, the review points out redundant code in Palace material resolution and frequency step calculations, as well as an incorrect validity range for the SiO2 model.
| "print(sim.validate_config())" | ||
| "print(sim.validate_config())\n", | ||
| "\n", | ||
| "xsxs" |
There was a problem hiding this comment.
This line appears to be leftover debugging code. It will cause a NameError when the notebook is executed and should be removed.
|
|
||
| print(sim.validate_config()) | ||
|
|
||
| xsxs |
There was a problem hiding this comment.
This line appears to be leftover debugging code. It will cause a NameError when the script is executed and should be removed.
| "# 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.
This line appears to be leftover debugging code. It will cause a NameError when the notebook is executed and should be removed.
Comment on lines
+70
to
+71
| mat_type = entry.get("type", "dielectric") | ||
| props_kwargs: dict = {"type": mat_type} |
There was a problem hiding this comment.
The type field is being added to props_kwargs, but the MaterialProperties model no longer has a type attribute. This will cause a TypeError when MaterialProperties(**props_kwargs) is called on line 105. The type field should not be added to the keyword arguments.
Suggested change
| mat_type = entry.get("type", "dielectric") | |
| props_kwargs: dict = {"type": mat_type} | |
| props_kwargs: dict = {} |
| def n_squared_contribution(self, wavelength_um: float) -> float: | ||
| """Compute the Sellmeier term's contribution to n^2 at a given wavelength.""" | ||
| lam_sq = wavelength_um**2 | ||
| return self.B * lam_sq / (lam_sq - self.C) |
There was a problem hiding this comment.
If wavelength_um**2 is exactly equal to self.C, this calculation will raise a ZeroDivisionError. This can happen if the evaluation wavelength is exactly at a resonance pole of the Sellmeier model. This edge case should be handled to prevent the program from crashing, for example by checking for this condition and raising a more informative ValueError.
Comment on lines
+575
to
+576
| validity=ValidityRange(valid_wavelength=(0.21, 6.7)), | ||
| source="Malitson 1965 (meep: fused_quartz)", |
There was a problem hiding this comment.
There are a couple of issues with the SiO2 Sellmeier model definition:
- Incorrect validity range: The validity range for the Malitson 1965 data for fused silica is 0.21-3.71 µm, as used in MEEP's material library. The upper bound of 6.7 µm seems incorrect and could lead to inaccurate results if used for wavelengths outside the fitted range.
- Incomplete citation: The source
"Malitson 1965 (meep: fused_quartz)"is a bit sparse. For better reproducibility and clarity, it would be beneficial to provide the full citation, e.g.,"I. H. Malitson, J. Opt. Soc. Am. 55, 1205 (1965)".
Suggested change
| validity=ValidityRange(valid_wavelength=(0.21, 6.7)), | |
| source="Malitson 1965 (meep: fused_quartz)", | |
| validity=ValidityRange(valid_wavelength=(0.21, 3.71)), | |
| source="I. H. Malitson, J. Opt. Soc. Am. 55, 1205 (1965)", |
| resolved[name] = dict(props) | ||
| continue | ||
|
|
||
| evaluated = resolve_material_at_wavelength(name, wavelength_um) |
There was a problem hiding this comment.
resolve_material_at_wavelength is called twice here with the same arguments as on line 50. This second call is redundant and can be removed.
| @@ -136,6 +141,11 @@ def validate_frequency_range(self) -> Self: | |||
| def to_palace_config(self) -> dict: | |||
| """Convert to Palace JSON config format.""" | |||
| freq_step = (self.fmax - self.fmin) / max(1, self.num_points - 1) / 1e9 | |||
There was a problem hiding this comment.
This line calculates freq_step, but the value is immediately re-calculated inside the following if/else block. This initial calculation is redundant and can be removed.
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/mesh-improvements
branch
from
e6e72cc to
f5e6c68
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #145 +/- ##
==========================================
+ Coverage 55.55% 57.29% +1.73%
==========================================
Files 59 61 +2
Lines 6966 7683 +717
Branches 1270 1453 +183
==========================================
+ Hits 3870 4402 +532
- Misses 2697 2850 +153
- Partials 399 431 +32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Plumbs decimate_tolerance through extract_geometry, generate_mesh, preview, and mesh() so callers can reduce vertex count on curved geometry before meshing.
occ.cut(removeObject=True) destroys all object tags including those shared with previously processed entities from the fragment step. PEC block surfaces shared tags with metal surfaces; after cut destroyed them, subsequent cuts referenced invalid tags -> "Unknown OpenCASCADE entity" crash. Prune destroyed tags from processed_in_dim after each cut and retry with valid-only dimtags if cut fails.
Gmsh OCC verbosity was hardcoded to 3 (very chatty). Default now 0 (silent). Users can opt in via sim.mesh(gmsh_verbosity=N).
benvial
force-pushed
the
feat/mesh-improvements
branch
from
f5e6c68 to
3e14df2
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
decimate_toleranceparam for polygon simplification in mesh generationverbosityparam tomesh()for controlling gmsh OCC output (default 0 = silent)gmsh_verbosity→verbosityfor cleaner APIDependencies
Depends on PR #143 (frequency-dependent materials) — mesh code touches material resolution.