Skip to content

Conversation

@marc-flex
Copy link
Contributor

@marc-flex marc-flex commented Dec 5, 2025

Greptile Overview

Greptile Summary

This PR refactors the simulation type detection logic to make analysis_spec the authoritative source for determining CHARGE simulation type, rather than inferring it from semiconductor presence.

Key changes:

  • Moved ChargeTypes and ChargeMonitorTypes to module level for reuse across methods
  • Updated _check_simulation_types to trigger CHARGE only when analysis_spec is a ChargeTypes instance
  • Added validation in check_charge_simulation to ensure semiconductors are present when CHARGE simulation is detected
  • Simplified _get_simulation_types to directly return CHARGE type based on analysis_spec
  • Fixed typos: "foop" → "loop", "structrures" → "structures", "additionaly" → "additionally"
  • Added comprehensive test validating that semiconductors alone don't trigger CHARGE simulation

The logic is now more explicit and predictable: CHARGE simulations require both a ChargeTypes analysis_spec AND semiconductor media, with proper validation ensuring both conditions are met.

Confidence Score: 4/5

  • This PR is safe to merge with the logic refactoring properly validated by tests
  • The refactoring clarifies simulation type detection logic and adds important validation. The changes are well-tested and address previous review concerns about MultiPhysicsMedium handling. Minor deduction for typo fixes being bundled with logic changes in a "hotfix" branch
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/tcad/simulation/heat_charge.py 4/5 Refactored simulation type checking logic to rely on analysis_spec rather than semiconductor presence, moved type definitions to module level, added semiconductor validation for charge simulations, and fixed typos
tests/test_components/test_heat_charge.py 5/5 Added comprehensive test to verify heat-only simulations with semiconductors don't incorrectly trigger charge simulation type

Sequence Diagram

sequenceDiagram
    participant User
    participant HeatChargeSimulation
    participant _check_simulation_types
    participant _check_if_semiconductor_present
    participant check_charge_simulation

    User->>HeatChargeSimulation: Create simulation with analysis_spec
    HeatChargeSimulation->>_check_simulation_types: Validate (root validator)
    
    alt analysis_spec is ChargeTypes
        _check_simulation_types->>_check_simulation_types: Add CHARGE to simulation_types
    end
    
    _check_simulation_types->>_check_if_semiconductor_present: Check for semiconductors
    _check_if_semiconductor_present-->>_check_simulation_types: semiconductor_present flag
    
    loop For each boundary
        alt Boundary is HeatBCTypes
            _check_simulation_types->>_check_simulation_types: Add HEAT to simulation_types
        else Boundary is ElectricBCTypes AND no semiconductors
            _check_simulation_types->>_check_simulation_types: Add CONDUCTION to simulation_types
        end
    end
    
    loop For each source
        alt Source is HeatSourceTypes
            _check_simulation_types->>_check_simulation_types: Add HEAT to simulation_types
        end
    end
    
    _check_simulation_types-->>HeatChargeSimulation: Return simulation_types
    
    HeatChargeSimulation->>check_charge_simulation: Validate charge setup
    
    alt CHARGE in simulation_types
        check_charge_simulation->>check_charge_simulation: Validate VoltageBC count >= 2
        check_charge_simulation->>check_charge_simulation: Validate ChargeMonitorTypes present
        check_charge_simulation->>_check_if_semiconductor_present: Check semiconductors
        _check_if_semiconductor_present-->>check_charge_simulation: semiconductor_present
        alt No semiconductors
            check_charge_simulation->>User: Raise SetupError
        end
    end
    
    check_charge_simulation-->>HeatChargeSimulation: Validation complete
    HeatChargeSimulation-->>User: Simulation created successfully
Loading

@marc-flex marc-flex marked this pull request as ready for review December 5, 2025 10:56
Copilot AI review requested due to automatic review settings December 5, 2025 10:56
Copilot finished reviewing on behalf of marc-flex December 5, 2025 10:59
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.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the simulation type checking logic for TCAD charge simulations. The main purpose is to change how charge simulation types are determined - now primarily based on analysis_spec type rather than the presence of semiconductor materials, with additional validation to ensure semiconductors are present when required.

Key Changes:

  • Moved ChargeTypes and ChargeMonitorTypes tuple definitions from local scope to module level for better reusability
  • Modified _check_simulation_types to determine CHARGE type based on analysis_spec instead of semiconductor presence
  • Added validation in check_charge_simulation to ensure at least one semiconductor medium exists in charge simulations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2025

Diff Coverage

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

  • tidy3d/components/tcad/simulation/heat_charge.py (100%)

Summary

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

@damiano-flex damiano-flex force-pushed the hotfix/charge_simulation_type_update branch from 09478f6 to d0abb8a Compare December 5, 2025 12:15
@damiano-flex
Copy link
Contributor

@greptile Check the latest version of this PR. Check that previous issues have been resolved.

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.

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

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.

3 participants