Skip to content

Conversation

weiliangjin2021
Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 commented Sep 27, 2025

Greptile Overview

Updated On: 2025-09-27 19:07:10 UTC

Summary

This PR improves the robustness of material dispersion models by addressing edge cases in Sellmeier, Debye, and PoleResidue models.

The key improvements include:

  • PoleResidue validation: Added bounds checking to prevent excessively large pole parameters using the new LARGEST_FP_NUMBER constant
  • Sellmeier robustness: When coefficient C ≈ 0, the contribution is now added to eps_inf instead of creating problematic poles
  • Debye robustness: When tau is very small (< 1/(2π·LARGEST_FP_NUMBER)), the contribution is added to eps_inf to avoid numerical instability
  • Mixed parameter validation: Added validators to prevent mixing zero/near-zero and normal values within coefficient arrays
  • Test updates: Modified test coefficients from 0 to 0.1 to avoid the edge case behaviors being handled

The changes follow good engineering practices by gracefully handling numerical edge cases that could cause instability or precision issues in electromagnetic simulations.

Confidence Score: 4/5

  • This PR is generally safe to merge with one logic bug that should be fixed
  • Score reflects solid robustness improvements and good validation logic, but reduced by one critical bug in the Debye model where eps_inf is incorrectly scoped within the loop
  • tidy3d/components/medium.py requires attention due to the eps_inf scoping bug in the Debye model

Important Files Changed

File Analysis

Filename        Score        Overview
tidy3d/components/medium.py 4/5 Enhanced robustness for Sellmeier/Debye models and added PoleResidue validation, but has one logic bug in Debye implementation
tests/test_components/test_custom.py 5/5 Updated test values from 0 to 0.1 for tau and C coefficients to avoid edge case behaviors
tidy3d/constants.py 5/5 Added LARGEST_FP_NUMBER constant (1e38) for single precision floating point validation
CHANGELOG.md 5/5 Added clear changelog entry describing robustness improvements to material models

Sequence Diagram

sequenceDiagram
    participant User as User/Application
    participant PR as PoleResidue
    participant SM as Sellmeier
    participant DB as Debye
    participant Val as Validators
    participant Const as Constants

    User->>PR: Create PoleResidue with poles
    PR->>Val: Validate causality (Re(a_i) ≤ 0)
    PR->>Const: Check LARGEST_FP_NUMBER
    PR->>Val: Validate pole magnitudes < LARGEST_FP_NUMBER
    Val-->>PR: Validation complete
    PR-->>User: PoleResidue created safely

    User->>SM: Create Sellmeier with coeffs (B, C)
    SM->>Val: Check if C ≈ 0
    alt C ≈ 0
        SM->>SM: Add B to eps_inf instead of creating pole
    else C >> 0
        SM->>SM: Create pole (a=jβ, c=jα)
    end
    SM->>Val: Validate mixed zero/non-zero C values
    Val-->>SM: Validation complete
    SM-->>User: Sellmeier created robustly

    User->>DB: Create Debye with coeffs (de, tau)
    DB->>Const: Check tau vs 1/(2π·LARGEST_FP_NUMBER)
    alt tau very small
        DB->>DB: Add de to eps_inf instead of creating pole
    else tau normal
        DB->>DB: Create pole (a=-2π/tau, c=-0.5·de·a)
    end
    DB->>Val: Validate mixed small/normal tau values
    Val-->>DB: Validation complete
    DB-->>User: Debye created robustly
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@weiliangjin2021
Copy link
Collaborator Author

weiliangjin2021 commented Sep 28, 2025

@dbochkov-flexcompute could you help take a look at the failed tests? I added a validation that all tau_i in CustomDebye should be sufficently larger than 0 (around >1e-38). The one in utils.py seems to >1 already Figured it out.

Copy link
Contributor

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

  • tidy3d/components/medium.py (100%)
  • tidy3d/constants.py (100%)

Summary

  • Total: 37 lines
  • Missing: 0 lines
  • Coverage: 100%

Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, similarly to most other pole residue constraints, it's not easy to impose them in fitting, right?

@weiliangjin2021
Copy link
Collaborator Author

I guess, similarly to most other pole residue constraints, it's not easy to impose them in fitting, right?

All material library ones pass. @caseyflex could yo comment on the fitter?

@caseyflex
Copy link
Contributor

I guess, similarly to most other pole residue constraints, it's not easy to impose them in fitting, right?

All material library ones pass. @caseyflex could yo comment on the fitter?

Not sure. For constraints on the poles, you could probably just move the poles during the relocation step. Constraints on the residues might be trickier.

@momchil-flex momchil-flex added this pull request to the merge queue Oct 1, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 1, 2025
@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/medium branch 2 times, most recently from 176467f to 4b1515e Compare October 1, 2025 13:27
@weiliangjin2021 weiliangjin2021 added this pull request to the merge queue Oct 1, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 1, 2025
@weiliangjin2021 weiliangjin2021 added this pull request to the merge queue Oct 1, 2025
@daquinteroflex daquinteroflex removed this pull request from the merge queue due to a manual request Oct 1, 2025
@weiliangjin2021 weiliangjin2021 added this pull request to the merge queue Oct 1, 2025
Merged via the queue into develop with commit 6f783bd Oct 1, 2025
21 checks passed
@weiliangjin2021 weiliangjin2021 deleted the weiliang/medium branch October 1, 2025 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants