Skip to content

Conversation

@marc-flex
Copy link
Contributor

@marc-flex marc-flex commented Nov 19, 2025

Greptile Summary

  • Fixed doping_bounds() to return [0, 0] instead of sentinel values [1e50, -1e50] when no doping structures are present
  • This prevents invalid visualization limits when scenes lack semiconductor doping configurations

Confidence Score: 3/5

  • This PR fixes a visualization bug but uses direct float equality which could fail in edge cases
  • The fix correctly handles the empty doping case but violates the floating-point comparison best practice (custom rule 4a0cb6d3). Direct equality == 1e50 should use np.isclose() for robustness. The logic is sound but the implementation needs a minor correction.
  • Pay attention to tidy3d/components/scene.py - the sentinel value checks need tolerance-based comparison

Important Files Changed

Filename Overview
tidy3d/components/scene.py Added sentinel value checks to handle empty doping case, but uses direct float equality instead of tolerance-based comparison

Sequence Diagram

sequenceDiagram
    participant User
    participant Scene
    participant doping_bounds
    participant Structures

    User->>Scene: "Visualize doping properties (N_a, N_d, or doping)"
    Scene->>doping_bounds: "Calculate doping bounds"
    doping_bounds->>Structures: "Loop through all_structures"
    alt Structures with SemiconductorMedium found
        Structures-->>doping_bounds: "Update acceptors_lims and donors_lims"
    else No structures with doping
        Structures-->>doping_bounds: "Limits remain at sentinel values [1e50, -1e50]"
    end
    doping_bounds->>doping_bounds: "Check if limits are still at sentinel values"
    alt Limits are sentinel values
        doping_bounds->>doping_bounds: "Set limits to [0, 0]"
    end
    doping_bounds-->>Scene: "Return acceptors_lims, donors_lims"
    Scene-->>User: "Use limits for visualization"
Loading

Copilot AI review requested due to automatic review settings November 19, 2025 10:55
Copilot finished reviewing on behalf of marc-flex November 19, 2025 10:58
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 hotfix addresses an issue where the doping_bounds() method would return invalid sentinel values ([1e50, -1e50]) when no doping structures are present in a scene, instead of returning sensible default values of [0, 0].

Key Changes:

  • Added sentinel value checks after the doping bounds calculation loop
  • Reset bounds to [0, 0] when no doping values are found in the scene

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

@marc-flex marc-flex self-assigned this Nov 19, 2025
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
React with 👍 or 👎 to share your feedback on this new summary format

@marc-flex marc-flex force-pushed the hotfix/fix_scene_doping_limits branch from c09ccc0 to 9b4bbcb Compare November 19, 2025 11:15
@github-actions
Copy link
Contributor

github-actions bot commented Nov 19, 2025

Diff Coverage

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

  • tidy3d/components/scene.py (60.0%): Missing lines 2025,2027,2029,2031

Summary

  • Total: 10 lines
  • Missing: 4 lines
  • Coverage: 60%

tidy3d/components/scene.py

  2021                                 if max_value > limits[1]:
  2022                                     limits[1] = max_value
  2023         # make sure we have recorded some values. Otherwise, set to 0
  2024         if np.isinf(acceptors_lims[0]):
! 2025             acceptors_lims[0] = 0
  2026         if np.isinf(acceptors_lims[1]):
! 2027             acceptors_lims[1] = 0
  2028         if np.isinf(donors_lims[0]):
! 2029             donors_lims[0] = 0
  2030         if np.isinf(donors_lims[1]):
! 2031             donors_lims[1] = 0
  2032         return acceptors_lims, donors_lims
  2033 
  2034     def doping_absolute_minimum(self):
  2035         """Get the absolute minimum values of the doping concentrations.

@marc-flex marc-flex force-pushed the hotfix/fix_scene_doping_limits branch from 0916999 to 0b3184d Compare November 19, 2025 14:14
@marc-flex marc-flex enabled auto-merge November 19, 2025 14:15
@marc-flex marc-flex added this pull request to the merge queue Nov 19, 2025
Merged via the queue into develop with commit ac9970b Nov 19, 2025
26 checks passed
@marc-flex marc-flex deleted the hotfix/fix_scene_doping_limits branch November 19, 2025 15:26
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