Skip to content

Conversation

@marc-flex
Copy link
Contributor

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

Currently we have basically two options of platting our simulation object for HeatChargeSimulation object that involve using the property argument in the plot_property function:

  • property=”heat_conductivity” → plots the structures greyed by thermal conductivity and shows thermal BCs
  • property=”electric_conductivity” → plots structures greyed by electric conductivity and shows electric BCs

In the case of Charge simulations, we don’t have a value for conductivity in the SemiconductorMedium and so the function fails to plot the strucutres.

The idea is to include a new property that allows us to plot the structures and show electric BCs

Greptile Overview

Greptile Summary

Adds a new "charge" property option to plot_property() in HeatChargeSimulation, allowing visualization of charge simulations that use SemiconductorMedium (which lacks a conductivity value). This addresses a limitation where property="electric_conductivity" would fail for such simulations.

  • Extends plot_property(), plot_boundaries(), and plot_sources() to handle "charge" property
  • Renders charge simulations with structures in gray fill with black edges (no conductivity-based grayscale)
  • Disables colorbar for charge plots since there's no conductivity value to display
  • Shows electric boundary conditions and charge sources when property="charge"
  • Refactors charge_tolerance fixture from class-level to module-level for broader test reuse
  • Missing CHANGELOG entry for this user-facing feature (per custom rule)

Confidence Score: 4/5

  • This PR is safe to merge - straightforward feature addition with proper validation and test coverage.
  • The implementation is well-structured with appropriate guards for simulation type validation, clear branching logic, and comprehensive test coverage including negative cases. Minor concern: dead code in _get_structure_heat_charge_property_plot_params and missing CHANGELOG entry.
  • No files require special attention. The implementation is consistent across all modified files.

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/tcad/simulation/heat_charge.py 4/5 Adds "charge" property support to plot_property() and related methods (plot_boundaries, plot_sources, source_bounds), enabling visualization of charge simulations with appropriate boundary conditions and sources.
tidy3d/components/scene.py 4/5 Adds "charge" property to type hints and handles the new property in plot_heat_charge_property, heat_charge_property_bounds, and related methods with appropriate styling (gray fill, black edge).
tests/test_components/test_heat_charge.py 5/5 Adds test_plot_property_charge test and refactors charge_tolerance fixture from class-level to module-level scope for broader reuse.

Sequence Diagram

sequenceDiagram
    participant User
    participant HeatChargeSimulation
    participant Scene
    participant PlotMethods

    User->>HeatChargeSimulation: plot_property(property="charge", z=0)
    HeatChargeSimulation->>HeatChargeSimulation: _get_simulation_types()
    
    alt CHARGE not in simulation_types
        HeatChargeSimulation-->>User: ValueError
    end
    
    HeatChargeSimulation->>Scene: plot_heat_charge_property(property="charge")
    Scene->>Scene: heat_charge_property_bounds(property="charge")
    Scene-->>Scene: return (0, 1)
    
    loop for each structure
        Scene->>Scene: _plot_shape_structure(property="charge")
        Scene->>Scene: _get_structure_plot_params(property="charge")
        Note over Scene: Apply edgecolor="k", linewidth=1
    end
    
    HeatChargeSimulation->>PlotMethods: plot_sources(property="charge")
    Note over PlotMethods: Filter ChargeSourceTypes
    
    HeatChargeSimulation->>PlotMethods: plot_boundaries(property="charge")
    Note over PlotMethods: Filter ElectricBCTypes
    
    HeatChargeSimulation-->>User: matplotlib.axes
Loading

Context used:

  • Rule from dashboard - Require a changelog entry for any PR that is not purely an internal refactor. (source)

@marc-flex marc-flex self-assigned this Dec 4, 2025
@marc-flex marc-flex changed the title Adding plot function for charge feat(FXC-4425) Adding plot function for charge Dec 4, 2025
@marc-flex marc-flex marked this pull request as ready for review December 4, 2025 14:57
Copilot AI review requested due to automatic review settings December 4, 2025 14:57
Copilot finished reviewing on behalf of marc-flex December 4, 2025 15:01
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 adds support for plotting charge simulations by introducing a new property="charge" option to the plot_property function. Previously, plotting charge simulations with SemiconductorMedium failed because these mediums don't have a conductivity value. The new option plots structures with fixed styling (lightgray fill with black borders) and displays electric boundary conditions.

Key Changes:

  • Added "charge" as a valid property option for plotting functions
  • Implemented special handling for charge simulations that bypasses conductivity-based coloring
  • Added validation to ensure property="charge" is only used with charge simulations

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
tidy3d/components/tcad/simulation/heat_charge.py Updated plot_property to accept "charge" property, disable colorbar for charge, add validation logic, and filter boundaries/sources appropriately for charge simulations
tidy3d/components/scene.py Added "charge" property handling in plotting functions with type hints, custom plot parameters (lightgray/black), and property bounds
tests/test_components/test_heat_charge.py Added test coverage for the new charge plotting functionality and refactored charge_tolerance fixture to module scope
Comments suppressed due to low confidence (1)

tidy3d/components/scene.py:1722

  • When property="charge" and cbar=True, a colorbar will be added with an empty label. Since the "charge" property doesn't use grayscale coloring (it uses fixed lightgray), the colorbar is not meaningful. Consider adding a condition to skip the colorbar when property == "charge":
if cbar and property != "charge":
    # ... existing colorbar code
        if cbar:
            label = ""
            if property == "heat_conductivity":
                label = f"Thermal conductivity ({THERMAL_CONDUCTIVITY})"
            elif property == "electric_conductivity":
                label = f"Electric conductivity ({CONDUCTIVITY})"
            self._add_cbar(
                vmin=property_val_min,
                vmax=property_val_max,
                label=label,
                cmap=STRUCTURE_HEAT_COND_CMAP,
                ax=ax,
            )

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

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.

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2025

Diff Coverage

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

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

Summary

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

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.

2 participants