Skip to content

Conversation

@groberts-flex
Copy link
Contributor

@groberts-flex groberts-flex commented Dec 2, 2025

@yaugenst-flex , this is the error I was mentioning this morning. I was going to include it in my boundary handling PR but just in case that takes a little bit to fully get in, I figured it was worth having this fix in sooner since it's needed for correct PEC handling.

Greptile Overview

Greptile Summary

This PR fixes a critical bug in the Box geometry's shape gradient computation where the normal vector for the minimum face was incorrectly pointing inward (+1) instead of outward (-1). The evaluate_gradient_at_points method requires outward-pointing normals for correct PEC (Perfect Electric Conductor) boundary handling in adjoint optimization.

Key changes:

  • Modified tidy3d/components/geometry/base.py:2743 to conditionally set normal direction based on min_max_index
  • When min_max_index == 0 (minimum face), normal now points in -1 direction (outward from lower bound)
  • When min_max_index == 1 (maximum face), normal continues to point in +1 direction (outward from upper bound)
  • Added appropriate changelog entry under "Fixed" section describing the impact

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk - it's a targeted bug fix with clear correctness improvement
  • The fix is minimal, mathematically correct, and addresses a clear bug. The change correctly implements outward-pointing normals as required by the API documentation. The logic is straightforward: minimum faces need negative normals, maximum faces need positive normals. No other code paths are affected.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
CHANGELOG.md 5/5 Added clear, user-facing changelog entry describing the bug fix for Box shape gradient normal direction
tidy3d/components/geometry/base.py 5/5 Corrected normal vector direction for Box minimum face to point outward (-1 direction) as required by shape gradient API

Sequence Diagram

sequenceDiagram
    participant Client as Adjoint Client
    participant BoxFaces as Box._derivative_faces
    participant BoxFace as Box._derivative_face
    participant Eval as DerivativeInfo.evaluate_gradient_at_points
    
    Client->>BoxFaces: Compute shape gradients
    BoxFaces->>BoxFaces: Loop over min_max_index
    BoxFaces->>BoxFace: Call with min_max_index and axis_normal
    
    BoxFace->>BoxFace: Get coord from bounds_normal[min_max_index]
    BoxFace->>BoxFace: Build grid_points on face
    
    Note over BoxFace: Critical Fix: Set normal direction
    alt min_max_index equals 0
        BoxFace->>BoxFace: Set normals[:, axis_normal] = -1
    else min_max_index equals 1
        BoxFace->>BoxFace: Set normals[:, axis_normal] = +1
    end
    
    BoxFace->>Eval: Call with spatial_coords and normals
    Note over Eval: Requires outward-pointing normals for correct PEC handling
    Eval-->>BoxFace: Return gradient values
    
    BoxFace->>BoxFace: Compute vjp_value with integration_weight
    BoxFace-->>BoxFaces: Return vjp_value
    BoxFaces-->>Client: Return vjp_faces array
Loading

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

@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2025

Diff Coverage

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

  • tidy3d/components/geometry/base.py (100%)

Summary

  • Total: 1 line
  • Missing: 0 lines
  • Coverage: 100%

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.

2 participants