Skip to content

Conversation

@marc-flex
Copy link
Contributor

@marc-flex marc-flex commented Sep 30, 2025

  • Added Selberherr's model

Greptile Overview

Updated On: 2025-10-21 11:28:16 UTC

Greptile Summary

This PR adds support for impact ionization in semiconductor device simulations by implementing Selberherr's model as a new SelberherrImpactIonization class. Impact ionization is a critical physical phenomenon where high-energy carriers create electron-hole pairs through collisions, essential for modeling avalanche photodiodes and breakdown effects. The implementation follows established patterns for generation-recombination models in the TCAD module, with six material-dependent parameters (alpha_n_inf, alpha_p_inf, E_n_crit, E_p_crit, beta_n, beta_p) and two formulation options ('Selberherr' and 'PQ'). The new model is properly integrated into the type system, exposed through the public API, documented in the changelog, and covered by a basic construction test.

Changed Files
Filename Score Overview
CHANGELOG.md 5/5 Added changelog entry documenting the new Selberherr impact ionization model
tidy3d/components/tcad/types.py 5/5 Registered SelberherrImpactIonization in the import list and RecombinationModelType union
tidy3d/init.py 5/5 Exposed SelberherrImpactIonization in the public API by adding it to imports and __all__
tidy3d/components/tcad/generation_recombination.py 3/5 Implemented the core SelberherrImpactIonization class with parameters, docstring, and mathematical formulation
tests/test_components/test_heat_charge.py 4/5 Added basic construction test for the new model with sample parameters

Confidence score: 3/5

  • This PR adds new functionality but has validation and potential numerical stability concerns that need addressing before merge
  • Score reflects missing validation on the formulation field (should be restricted to 'Selberherr' or 'PQ'), potential division-by-zero in the mathematical formula, and insufficient test coverage for edge cases and validation
  • Pay close attention to tidy3d/components/tcad/generation_recombination.py (lines 357-362 for formulation validation and lines 295-299 for the mathematical formula), and tests/test_components/test_heat_charge.py (insufficient coverage)

Specific issues:

  1. Missing formulation validation (tidy3d/components/tcad/generation_recombination.py, line 357): The formulation field accepts any string but should be restricted to 'Selberherr' or 'PQ'. Use Pydantic's Literal["Selberherr", "PQ"] type annotation instead of str to enforce valid values at model construction time.

  2. Potential numerical instability (tidy3d/components/tcad/generation_recombination.py, line 299): The mathematical formula involves E · J_ν in the denominator, which could be zero or negative if the electric field and current density are perpendicular or opposing. Consider adding documentation about expected input ranges or runtime validation to prevent undefined behavior.

  3. Insufficient test coverage (tests/test_components/test_heat_charge.py, lines 2386-2394): The test only verifies successful construction with valid parameters. Add tests for: (a) invalid formulation strings to verify validation works, (b) boundary cases for numerical parameters (zeros, very small/large values), and (c) integration with charge simulations if applicable. (Rule: 40c78813-75a7-47ea-a080-1509e6e686a9)

  4. Incomplete propagation check: Verify that the new model is properly handled in all code paths that process RecombinationModelType. Search for pattern matches, isinstance checks, or method dispatch on recombination models to ensure nothing was missed. (Rule: 809457d4-6b33-46ed-b49b-37057d02807e)

Sequence Diagram

sequenceDiagram
    participant User
    participant SemiconductorMedium
    participant HeatChargeSimulation
    participant Solver

    User->>SemiconductorMedium: "Create with SelberherrImpactIonization in R parameter"
    SemiconductorMedium-->>User: "Returns configured medium"
    User->>HeatChargeSimulation: "Create simulation with semiconductor structures"
    HeatChargeSimulation->>HeatChargeSimulation: "Validate configuration"
    User->>Solver: "web.run(simulation)"
    Solver->>Solver: "Apply impact ionization model during solve"
    Solver-->>User: "Returns simulation data"
Loading

Context used:

  • Rule from dashboard - When modifying a piece of logic, ensure the change is propagated to all independent functions or cal... (source)
  • Rule from dashboard - Add tests for all public constructors and methods to confirm expected behavior, including effects on... (source)

@marc-flex marc-flex self-assigned this Sep 30, 2025
* Added Selberherr's model
@marc-flex marc-flex force-pushed the marc/impact_ionization branch from f33dcfa to 301c0c8 Compare October 21, 2025 11:26
@marc-flex marc-flex marked this pull request as ready for review October 21, 2025 11:27
@marc-flex marc-flex merged commit e3c0878 into marc/btbt_direct Oct 21, 2025
@marc-flex marc-flex deleted the marc/impact_ionization branch October 21, 2025 11:27
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.

5 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +357 to +362
formulation: str = pd.Field(
"PQ",
title="Formulation",
description="Formulation used for impact ionization. Options are 'Selberherr' "
"or 'PQ' for Selberherr and Palankovski and Quay formulations, respectively.",
)
Copy link

Choose a reason for hiding this comment

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

logic: The formulation field should have validation to ensure only 'Selberherr' or 'PQ' are accepted. Use Literal['Selberherr', 'PQ'] or add a validator to prevent invalid values.

Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/components/tcad/generation_recombination.py
Line: 357:362

Comment:
**logic:** The `formulation` field should have validation to ensure only 'Selberherr' or 'PQ' are accepted. Use `Literal['Selberherr', 'PQ']` or add a validator to prevent invalid values.

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant