Skip to content

Conversation

@marc-flex
Copy link
Contributor

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

Greptile Summary

This PR enhances HeatChargeSimulation.plot() to automatically display boundary conditions based on the simulation type. When called, the method now determines whether it's a HEAT, CHARGE, or CONDUCTION simulation and adds the appropriate boundary condition visualizations (heat boundaries for HEAT simulations, electric boundaries for CHARGE/CONDUCTION simulations).

Key changes:

  • Overrides plot() method in HeatChargeSimulation to call parent's implementation then add simulation-type-specific boundary conditions
  • Delegates to existing plot_boundaries() method with appropriate property parameter
  • Added Literal type hint for property parameter in plot_property() method
  • Comprehensive test coverage added for both HEAT and CHARGE simulation plotting scenarios

Issues found:

  • Missing **patch_kwargs parameter from parent class signature - breaks backward compatibility if users pass additional keyword arguments to plot()
  • The **patch_kwargs parameter needs to be forwarded to super().plot() call

Confidence Score: 4/5

  • This PR is generally safe to merge with one important fix needed for backward compatibility
  • The implementation is clean and well-tested, but the missing **patch_kwargs parameter creates a backward compatibility issue where users passing additional keyword arguments to plot() will encounter errors. This is a logical error that needs to be fixed before merging. The rest of the implementation follows good patterns by delegating to the parent class and reusing existing plot_boundaries() functionality.
  • Pay close attention to tidy3d/components/tcad/simulation/heat_charge.py - the plot() method signature must match the parent class to maintain backward compatibility

Important Files Changed

Filename Overview
CHANGELOG.md Added changelog entry for new boundary condition plotting feature
tests/test_components/test_heat_charge.py Added comprehensive test coverage for both HEAT and CHARGE simulation plotting with boundary conditions
tidy3d/components/tcad/simulation/heat_charge.py Implemented plot() override to automatically add boundary conditions; missing **patch_kwargs parameter from parent signature

Sequence Diagram

sequenceDiagram
    participant User
    participant HeatChargeSimulation
    participant AbstractSimulation
    participant Scene
    participant plot_boundaries

    User->>HeatChargeSimulation: plot(x, y, z, ax, ...)
    HeatChargeSimulation->>AbstractSimulation: super().plot(x, y, z, ax, ...)
    AbstractSimulation->>Scene: scene.plot_structures()
    Scene-->>AbstractSimulation: ax
    AbstractSimulation->>AbstractSimulation: plot_sources()
    AbstractSimulation->>AbstractSimulation: plot_monitors()
    AbstractSimulation->>AbstractSimulation: plot_boundaries()
    AbstractSimulation->>AbstractSimulation: plot_symmetries()
    AbstractSimulation-->>HeatChargeSimulation: ax
    HeatChargeSimulation->>HeatChargeSimulation: _get_simulation_types()
    alt HEAT simulation
        HeatChargeSimulation->>plot_boundaries: plot_boundaries(property="heat_conductivity")
        plot_boundaries-->>HeatChargeSimulation: ax
    end
    alt CHARGE or CONDUCTION simulation
        HeatChargeSimulation->>plot_boundaries: plot_boundaries(property="electric_conductivity")
        plot_boundaries-->>HeatChargeSimulation: ax
    end
    HeatChargeSimulation-->>User: ax
Loading

Note

Enhances visualization by adding boundary conditions to HeatChargeSimulation.plot() based on simulation type.

  • Override plot() to call parent and then add electric boundaries for CHARGE/CONDUCTION via plot_boundaries(property="electric_conductivity") (heat boundaries handled by parent)
  • Preserve extensibility by forwarding **patch_kwargs to super().plot()
  • Tighten plot_property signature to Literal["heat_conductivity", "electric_conductivity", "source"]
  • Add tests covering HEAT and CHARGE scenarios validating BCs and monitors appear in plots
  • Update CHANGELOG.md to note BC plotting behavior change

Written by Cursor Bugbot for commit 7a85bb5. This will update automatically on new commits. Configure here.

@marc-flex marc-flex self-assigned this Dec 18, 2025
@marc-flex marc-flex marked this pull request as ready for review December 18, 2025 14:17
Copilot AI review requested due to automatic review settings December 18, 2025 14:17
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 a new plot() method to the HeatChargeSimulation class that provides a unified plotting interface. When property is None, it plots scene structures using the parent class's plot method and automatically adds electric boundary conditions for CHARGE simulations. When property is specified, it delegates to the existing plot_property() method.

  • Added plot() method with optional property parameter for flexible visualization
  • Automatic plotting of electric boundary conditions for CHARGE simulations when property=None
  • Updated type annotations for plot_property() method to use Literal types for better type safety

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 Added new plot() method with unified interface, updated type annotations for plot_property() parameter, and added Literal import
tests/test_components/test_heat_charge.py Added comprehensive test coverage for the new plot() method covering HEAT and CHARGE simulations with various property values
CHANGELOG.md Documented the new feature in the unreleased section

💡 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.

Additional Comments (1)

  1. tidy3d/components/tcad/simulation/heat_charge.py, line 1284 (link)

    syntax: Docstring type should match type hint more precisely

    Context Used: Rule from dashboard - In docstrings, use the format name : type = default for parameter documentation instead of numpydo... (source)

3 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

@marc-flex marc-flex force-pushed the feat/FXC-4425/add_electric_bc_to_plot branch from bef8d70 to 98bbae7 Compare December 18, 2025 14:45
@marc-flex
Copy link
Contributor Author

marc-flex commented Dec 18, 2025

@greptile update summary

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, 2 comments

Edit Code Review Agent Settings | Greptile

@marc-flex marc-flex force-pushed the feat/FXC-4425/add_electric_bc_to_plot branch from 98bbae7 to f7df7a7 Compare December 18, 2025 15:11
@marc-flex marc-flex changed the title feat(FXC-4425) Plot electric BCs in Charge simulations feat(FXC-4425) Plot BCs in Heat/Charge simulations Dec 18, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Dec 18, 2025

Diff Coverage

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

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

Summary

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

@marc-flex marc-flex force-pushed the feat/FXC-4425/add_electric_bc_to_plot branch from f7df7a7 to de40e09 Compare January 9, 2026 09:50
@marc-flex marc-flex enabled auto-merge January 9, 2026 09:50
@marc-flex marc-flex force-pushed the feat/FXC-4425/add_electric_bc_to_plot branch 2 times, most recently from ca171cf to 88ef91c Compare January 9, 2026 12:33
@marc-flex marc-flex added this pull request to the merge queue Jan 9, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2026
@marc-flex marc-flex force-pushed the feat/FXC-4425/add_electric_bc_to_plot branch from 88ef91c to 7a85bb5 Compare January 9, 2026 14:24
@marc-flex marc-flex enabled auto-merge January 9, 2026 14:24
@marc-flex marc-flex added this pull request to the merge queue Jan 9, 2026
Merged via the queue into develop with commit 5f5f31a Jan 9, 2026
33 of 37 checks passed
@marc-flex marc-flex deleted the feat/FXC-4425/add_electric_bc_to_plot branch January 9, 2026 15:26
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