-
Notifications
You must be signed in to change notification settings - Fork 70
FXC-3763: Added more lossy dielectrics and lossy metals with frequency parametrization to RF material library #3255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
d3b2269 to
2617775
Compare
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
tidy3d/plugins/microwave/rf_material_library.pyLines 312-323 312 "where frequency_range is a tuple (f_min, f_max) in Hz."
313 )
314 else:
315 # Fallback: try to call if it's callable, otherwise return as-is
! 316 medium_attr = getattr(variant, "medium", None)
! 317 if callable(medium_attr):
! 318 return medium_attr()
! 319 return medium_attr
320
321
322 Rogers3003_design = VariantItem(
323 medium=PoleResidue( |
2617775 to
dd11696
Compare
8188acd to
6f4b8ac
Compare
|
Could you also update doc-related scripts and take a look at how the doc page looks like? |
|
A few comments by Claude first: tidy3d/plugins/microwave/rf_material_library.py:761 Megtron6_R5775_N.frequency_range appears too narrow This variant has measurement data spanning 1 GHz to 50 GHz (10 data points at lines 751-753), but Should this be (1e9, 5e10) to match the measurement range? (Compare with Megtron6_R5670_N at line tidy3d/plugins/microwave/rf_material_reference.py:133-138 Cobalt reference appears to cite the wrong paper The reference for Cobalt cites "Benon H. J. Bielski, et al., Reactivity of HO2/O Radicals in tidy3d/plugins/microwave/rf_material_library.py:780 "Neclo" should be "Nelco" The variable names and library keys (Neclo_N4000_6, Neclo_N4000_13EP, etc.) misspell the brand — tidy3d/plugins/microwave/rf_material_library.py:1060-1085 Duplicate display names across distinct library entries Several pairs share the same name field:
Could be confusing in listings/summaries. Consider differentiating the display names (e.g. tidy3d/plugins/microwave/rf_material_reference.py:90-91 Duplicated substring in datasheet_title Python implicit string concatenation produces: tidy3d/plugins/microwave/rf_material_library.py:314-319 Nit: dead else branch VariantItemFreqRange is sealed to VariantItemFreqRangeDielectric and VariantItemFreqRangeMetal — tidy3d/plugins/microwave/rf_material_library.py:1144-1150 Nit: Aluminum/Aluminium dual library entries Both "Aluminium" and "Aluminum" exist as separate entries pointing to the same variant object. |
| Returns the medium for the default variant using its default frequency range. | ||
| For dielectrics, this returns the stored PoleResidue model (equivalent to calling | ||
| ``variant.medium()`` without arguments). | ||
| For metals, raises ValueError as frequency_range is required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes me wonder if we should add a default frequency range to VariantItemFreqRangeMetal, which is based on measured data or known valid range. With that both Variant will have a default frequency range.
dmarek-flex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a nice contribution! Just a bit confused by that part where it falls back to checking if the variant is callable. From the Pydnatic model it seems like it has to be either one of the previous classes? VariantItemFreqRangeDielectric/VariantItemFreqRangeMetal
6f4b8ac to
f606b1e
Compare
f606b1e to
448e80b
Compare
yaugenst-flex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change introduces callable variant.medium(...) for RF frequency-range variants, but doc-generation logic still assumes var.medium is a medium object. Repro:
from tidy3d.plugins.microwave import rf_material_library as rf_lib
for _, mat in sorted(rf_lib.items()):
for _, var in sorted(mat.variants.items()):
medium = var.medium
_ = len(medium.poles)This fails with AttributeError: 'function' object has no attribute 'poles'. Could we update docs/generate_doc.py accordingly?
448e80b to
b3deca3
Compare
b3deca3 to
b065a27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
b065a27 to
ecee22f
Compare
Summary
Adds frequency-parametrized support for lossy dielectrics and lossy metals to the RF material library, enabling users to request material models for specific frequency ranges.
Changes
Core Functionality
Added frequency-parametrized materials:
VariantItemFreqRangeDielectric: For lossy dielectric materials withPoleResiduemodelsVariantItemFreqRangeMetal: For lossy metal materials withLossyMetalMediummodelsEnhanced
medium()methods:frequency_rangeparameterVariantItemFreqRangeDielectric: Validates if requested range is within stored measurementsPoleResiduemodel using constant loss tangent fitter with averaged properties when outside rangeVariantItemFreqRangeMetal: CreatesLossyMetalMediumfitted for the requested frequency rangeAdded fields to
VariantItemFreqRangeDielectric:loss_tangent: Loss tangent values (single or list)eps_real: Real permittivity values (single or list)measurement_frequencies: Frequencies at which measurements were takenTesting
AssertLogLevelBehavior Changes
Before: RF material library only supported fixed-frequency material models.
After: Users can now:
variant.medium()returns model for stored/default rangevariant.medium(frequency_range)returns model fitted for requested rangeTesting
Ran the following checks:
pytest tests/test_plugins/test_rf_material_library.py- All tests passDocumentation
VariantItemFreqRangeDielectricandVariantItemFreqRangeMetalNote
Medium Risk
Introduces new public material item/variant types and changes how some RF library entries are accessed/constructed (notably metals requiring
frequency_range), which could affect downstream usage and fitting accuracy for out-of-range requests.Overview
Adds frequency-range parametrization to the RF material library by introducing
VariantItemFreqRangeDielectricandVariantItemFreqRangeMetalplus a newMaterialItemFreqRangewrapper for these variants.Dielectric variants now accept measured
loss_tangent/eps_realdata and aprefitted_medium; requesting an out-of-rangefrequency_rangelogs a warning and fits a newPoleResidueusing averaged properties, while in-range requests reuse the prefitted model. Metal variants are added as conductivity-based entries that require an explicitfrequency_rangeto construct aLossyMetalMedium(with optional roughness/thickness/fit parameters).Expands the RF library catalog with multiple new dielectrics and metals, updates
rf_material_reference.pywith new sources, and adds a comprehensivetest_rf_material_library.pycovering validation, warning behavior, range handling, and material access patterns. Also updatesCHANGELOG.mdto document the new frequency-parameterized RF materials.Written by Cursor Bugbot for commit ecee22f. This will update automatically on new commits. Configure here.