Skip to content

fix(mode): fixed bug where mode solver was not respecting location of PEC#3030

Open
dmarek-flex wants to merge 3 commits intodevelopfrom
dmarek/FXC-4112-PEC-boundary-position-not-respected-by-ModeSolver
Open

fix(mode): fixed bug where mode solver was not respecting location of PEC#3030
dmarek-flex wants to merge 3 commits intodevelopfrom
dmarek/FXC-4112-PEC-boundary-position-not-respected-by-ModeSolver

Conversation

@dmarek-flex
Copy link
Contributor

@dmarek-flex dmarek-flex commented Nov 25, 2025

Greptile Overview

Greptile Summary

This PR fixes a bug where the mode solver grid would extend beyond simulation domain boundaries, causing incorrect PEC boundary condition application. The fix implements grid truncation at simulation boundaries and zero-pads fields outside the domain.

Key changes:

  • Added _sim_boundary_positions method to detect PEC boundaries and determine where to truncate the computational grid
  • Implemented truncation logic in compute_modes that clips coordinates, permittivity, permeability, and basis fields to simulation bounds
  • Added zero-padding to restore fields to original grid size after solving on truncated domain
  • Added warnings for unsupported boundary types (PMC, Periodic, Bloch) when mode plane intersects them
  • Added comprehensive tests verifying zero fields outside simulation and PEC boundary conditions

The implementation correctly handles the core issue, but there's a minor inconsistency in the boundary intersection detection logic that could cause edge cases.

Confidence Score: 4/5

  • This PR is generally safe to merge with one logical inconsistency that should be addressed
  • The implementation is well-designed with comprehensive tests, proper documentation, and correct changelog entry. However, there's a logical inconsistency in the boundary intersection detection (using np.isclose vs fp_eps threshold) that could cause edge case failures. The core truncation and zero-padding logic appears sound
  • Pay close attention to tidy3d/components/mode/mode_solver.py for the boundary intersection logic inconsistency

Important Files Changed

File Analysis

Filename Score Overview
CHANGELOG.md 5/5 Added changelog entry describing the bug fix for mode solver grid extending beyond simulation bounds
tests/test_plugins/test_mode_solver.py 4/5 Added comprehensive tests for PEC boundary truncation and zero-padding, including warning tests for unsupported boundary types. Tests verify field values outside simulation bounds and at PEC boundaries
tidy3d/components/mode/mode_solver.py 4/5 Added _sim_boundary_positions method to detect PEC boundaries and determine truncation positions. Passes boundary positions to solver for grid truncation. Minor inconsistency in boundary checking logic
tidy3d/components/mode/solver.py 4/5 Implements core truncation logic: truncates grid/eps/mu/fields to simulation bounds when needed, then zero-pads results. Adds _truncate_medium_data helper method

Sequence Diagram

sequenceDiagram
    participant MS as ModeSolver
    participant MSC as ModeSolver._solve_single_freq
    participant SBP as ModeSolver._sim_boundary_positions
    participant CM as compute_modes (solver.py)
    participant TMD as _truncate_medium_data
    
    MS->>MS: User calls .data property
    MS->>MSC: _solve_single_freq(freq, coords, symmetry)
    MSC->>SBP: Get PEC boundary positions
    SBP->>SBP: Check boundary types (PEC, PMC, etc.)
    SBP->>SBP: Determine if solver grid intersects boundaries
    SBP-->>MSC: Return (pos_min, pos_max) tuples
    
    MSC->>CM: compute_modes(eps_cross, coords, ..., sim_pec_pos_min, sim_pec_pos_max)
    
    CM->>CM: Calculate truncation indices from PEC positions
    CM->>CM: Check if truncation needed (trim_min != [0,0] or trim_max != [Nx,Ny])
    
    alt Truncation needed
        CM->>CM: Truncate coords arrays
        CM->>TMD: Truncate eps_cross
        TMD-->>CM: Truncated eps
        CM->>TMD: Truncate mu_cross (if present)
        TMD-->>CM: Truncated mu
        CM->>CM: Truncate split_curl_scaling (if present)
        CM->>CM: Truncate solver_basis_fields (if present)
    end
    
    CM->>CM: Solve eigenvalue problem on truncated grid
    CM->>CM: Reshape fields to (2, 3, Nx_trunc, Ny_trunc, 1, num_modes)
    
    alt Truncation was applied
        CM->>CM: Zero-pad fields back to original size
        Note over CM: np.pad with trim_min/trim_max offsets
    end
    
    CM-->>MSC: Return (n_complex, fields, eps_spec)
    MSC-->>MS: Return mode data with correct boundary conditions
Loading

Note

Medium Risk
Touches core mode-solver boundary handling and field interpolation/truncation logic; mistakes could subtly affect mode fields/impedances near boundaries despite added coverage via new regression tests.

Overview
Fixes a long-standing mismatch where ModeSolver could extend its solver grid beyond the simulation domain, causing PEC boundary conditions to be enforced at the wrong locations for interior ports.

ModeSolver now computes PEC positions by snapping the mode plane to the simulation grid, passes these bounds into compute_modes to truncate coordinates/material data (eps/mu/basis fields) during the eigensolve, and then zero-pads the solved fields back to the original shape. Colocation now slices fields to the PEC-bounded region and interpolates with fill_value="extrapolate".

Adds warnings when the mode plane intersects simulation boundaries that are not supported for the mode solver’s PEC enforcement (PMC/Periodic/Bloch), updates ModePlaneAnalyzer to filter conductors using the same PEC-snapped bounds (fixing ground-plane shorting behavior), introduces the Bound2D type, and expands tests to cover truncation/zero-padding, boundary warnings, interior-plane snapping, and degenerate-axis (2D) cases.

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

@dmarek-flex dmarek-flex self-assigned this Nov 25, 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.

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

github-actions bot commented Nov 25, 2025

Diff Coverage

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

  • tidy3d/components/mode/mode_solver.py (97.4%): Missing lines 1670
  • tidy3d/components/mode/solver.py (87.5%): Missing lines 167,171,1170,1172,1174
  • tidy3d/components/types/base.py (100%)

Summary

  • Total: 79 lines
  • Missing: 6 lines
  • Coverage: 92%

tidy3d/components/mode/mode_solver.py

Lines 1666-1674

  1666     ) -> tuple[list[float], list[dict[str, ArrayComplex4D]], list[EpsSpecType]]:
  1667         """Call the mode solver at all requested frequencies."""
  1668         if tidy3d_extras["use_local_subpixel"]:
  1669             subpixel_ms = tidy3d_extras["mod"].SubpixelModeSolver.from_mode_solver(self)
! 1670             return subpixel_ms._solve_all_freqs(
  1671                 coords=coords,
  1672                 symmetry=symmetry,
  1673             )

tidy3d/components/mode/solver.py

Lines 163-175

  163             eps_cross = cls._truncate_medium_data(eps_cross, cell_min, cell_max)
  164 
  165             # Truncate mu_cross if provided
  166             if mu_cross is not None:
! 167                 mu_cross = cls._truncate_medium_data(mu_cross, cell_min, cell_max)
  168 
  169             # Truncate split_curl_scaling if provided
  170             if split_curl_scaling is not None:
! 171                 split_curl_scaling = tuple(
  172                     s[cell_min[0] : cell_max[0], cell_min[1] : cell_max[1]]
  173                     for s in split_curl_scaling
  174                 )

Lines 1166-1178

  1166 
  1167         if isinstance(mat_data, Numpy):
  1168             # Tensorial format: shape (9, Nx, Ny)
  1169             return mat_data[:, x_slice, y_slice]
! 1170         if len(mat_data) == 9:
  1171             # Nine separate 2D arrays
! 1172             return tuple(arr[x_slice, y_slice] for arr in mat_data)
  1173 
! 1174         raise ValueError("Wrong input to mode solver permittivity/permeability truncation!")
  1175 
  1176     @staticmethod
  1177     def format_medium_data(mat_data):
  1178         """

@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-4112-PEC-boundary-position-not-respected-by-ModeSolver branch from f179f91 to 6bb0d8f Compare November 26, 2025 16:58
@dmarek-flex
Copy link
Contributor Author

@greptileai another round

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.

4 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-4112-PEC-boundary-position-not-respected-by-ModeSolver branch from d1ada2b to 3df4c17 Compare November 26, 2025 17:38
@weiliangjin2021
Copy link
Collaborator

Yes, so far I was trying to match existing behavior, but fix the bug. So if the mode plane is smaller than the Simulation plane bounds, there is no change in behavior when compared to develop. And if we choose the closest grid boundary, or say the closest grid boundary that encompasses the mode plane, then basically we have the same problem (adding additional space), just potentially not quite as bad as the one complete cell in the bug.

I think it's quite common to have mode plane smaller than the simulation domain, and it'll be very useful to fix that too?

Regarding closest grid boundary, i guess it should work in most cases, as it's expected to have the mode plane boundary to lie at the metal surface, and metal surface is snapped to grid.

@dmarek-flex
Copy link
Contributor Author

I think it's quite common to have mode plane smaller than the simulation domain, and it'll be very useful to fix that too?

It's not really an issue though if the mode plane is smaller than the simulation domain. All that will happen is an extra cell will be added that is outside the monitor bounds. But since in the simulation there is no PEC boundary at the monitor bounds anyways, it is an accurate representation. A structure just outside/touching the monitor bounds will still be captured by the mode solver, and is the current workaround for this bug.

Copy link
Contributor

@caseyflex caseyflex left a comment

Choose a reason for hiding this comment

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

I think the warnings make sense if the boundary won’t be used.

the only unclear case is PML, which you don’t earn for just because it would be annoying. I guess that can be a separate discussion about unifying PML treatment

@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-4112-PEC-boundary-position-not-respected-by-ModeSolver branch from 3df4c17 to 4384ea8 Compare December 12, 2025 22:26
@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-4112-PEC-boundary-position-not-respected-by-ModeSolver branch 7 times, most recently from a436028 to 5a716d5 Compare December 22, 2025 14:34
dmarek-flex and others added 3 commits February 23, 2026 12:50
… PEC boundary conditions from the Simulation
Replace `_sim_boundary_positions` with `_pec_boundary_positions` that
snaps PEC to the nearest grid boundary on or outside the mode plane
bounds using `snap_box_to_grid` with `SnapBehavior.Expand`. This fixes
interior wave ports where the mode plane is smaller than the simulation
domain — PEC is now placed at the plane edges instead of the distant
simulation edges, eliminating spurious air gaps.

Also add `_get_pec_boundary_positions` to `ModePlaneAnalyzer` so that
conductor filtering uses the same tight PEC bounds, fixing SCRF-2777
where ground planes were not being filtered as shorted to PEC.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-4112-PEC-boundary-position-not-respected-by-ModeSolver branch from 5a716d5 to b5eb390 Compare February 24, 2026 02:36
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

split_curl_scaling = tuple(
s[cell_min[0] : cell_max[0], cell_min[1] : cell_max[1]]
for s in split_curl_scaling
)
Copy link

Choose a reason for hiding this comment

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

Truncation converts split_curl_scaling array to tuple, breaking postprocessing

Medium Severity

The truncation code converts split_curl_scaling from a numpy array (shape (3, Nx, Ny)) into a Python tuple of three 2D arrays. Downstream, split_curl_field_postprocess accesses split_curl.shape which will raise AttributeError on a tuple. The truncation for this parameter needs to preserve the numpy array type, e.g., using split_curl_scaling[:, cell_min[0]:cell_max[0], cell_min[1]:cell_max[1]] instead of wrapping in tuple().

Additional Locations (1)

Fix in Cursor Fix in Web

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