Skip to content

Conversation

@dmarek-flex
Copy link
Contributor

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

Greptile Overview

Greptile Summary

This PR adds validation to ensure that port mesh refinement options are only used with compatible GridSpec types in TerminalComponentModeler. The validation prevents users from enabling mesh refinement features (like enable_snapping_points or num_grid_cells) when using UniformGrid, which doesn't support these features.

Key Changes:

  • Added _validate_port_refinement_usage() validator in TerminalComponentModeler that checks if ports have mesh refinement enabled when the simulation uses a non-snapping grid
  • Implemented _is_using_mesh_refinement property on both AbstractLumpedPort and WavePort to detect if refinement options are enabled
  • Updated tests to explicitly disable mesh refinement when using uniform grids (autograd tests) or switch to QuasiUniformGrid for wave port tests
  • Added test test_validate_port_refinement_with_uniform_grid() to verify the validation catches the incompatibility

The validation improves user experience by catching configuration errors early with a clear error message explaining how to fix the issue.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The changes are well-implemented with comprehensive test coverage, clear error messages following coding standards, and no breaking changes to existing functionality. The validation is purely additive and helps prevent configuration errors.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/plugins/smatrix/component_modelers/terminal.py 5/5 Adds validator to check port mesh refinement compatibility with GridSpec, preventing incompatible configurations
tidy3d/plugins/smatrix/ports/base_lumped.py 5/5 Adds property to check if lumped port uses mesh refinement options (snapping or custom cell count)
tidy3d/plugins/smatrix/ports/wave.py 5/5 Adds property to check if wave port uses mesh refinement via custom grid cell count
tests/test_plugins/smatrix/test_terminal_component_modeler.py 5/5 Updates tests to use QuasiUniformGrid for wave port tests and adds new validation test for uniform grid incompatibility

Sequence Diagram

sequenceDiagram
    participant User
    participant TerminalComponentModeler
    participant Port as AbstractPort
    participant GridSpec
    
    User->>TerminalComponentModeler: Create with simulation & ports
    TerminalComponentModeler->>TerminalComponentModeler: _validate_port_refinement_usage()
    TerminalComponentModeler->>GridSpec: Check snapped_grid_used
    alt Grid supports snapping (AutoGrid/QuasiUniform)
        GridSpec-->>TerminalComponentModeler: True
        TerminalComponentModeler-->>User: Validation passes
    else Uniform grid (no snapping support)
        GridSpec-->>TerminalComponentModeler: False
        loop For each port
            TerminalComponentModeler->>Port: Check _is_using_mesh_refinement
            alt LumpedPort with refinement
                Port-->>TerminalComponentModeler: enable_snapping_points=True OR num_grid_cells set
                TerminalComponentModeler-->>User: ValidationError
            else WavePort with refinement
                Port-->>TerminalComponentModeler: num_grid_cells set
                TerminalComponentModeler-->>User: ValidationError
            else No refinement options
                Port-->>TerminalComponentModeler: False
                TerminalComponentModeler-->>User: Validation passes
            end
        end
    end
Loading

@dmarek-flex dmarek-flex self-assigned this Nov 12, 2025
@dmarek-flex dmarek-flex added the RF label Nov 12, 2025
@dmarek-flex
Copy link
Contributor Author

@weiliangjin2021 I noticed that QuasiUniformGrid does not appear to use internal_snapping points. Is that an issue?

def internal_snapping_points(
self,
structures: list[Structure],
lumped_elements: list[LumpedElementType],
cached_corners_and_convexity=None,
) -> list[CoordinateOptional]:
"""Internal snapping points. So far, internal snapping points are generated by
`layer_refinement_specs` and lumped element.
Parameters
----------
structures : List[Structure]
List of physical structures.
lumped_elements : List[LumpedElementType]
List of lumped elements.
cached_corners_and_convexity : Optional[list[CachedCornersAndConvexity]]
Cached corners and convexity data.
Returns
-------
List[CoordinateOptional]
List of snapping points coordinates.
"""
# no need to generate anything if autogrid is not used
if not self.auto_grid_used:
return []
snapping_points = []

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.

7 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@weiliangjin2021
Copy link
Collaborator

@weiliangjin2021 I noticed that QuasiUniformGrid does not appear to use internal_snapping points. Is that an issue?

def internal_snapping_points(
self,
structures: list[Structure],
lumped_elements: list[LumpedElementType],
cached_corners_and_convexity=None,
) -> list[CoordinateOptional]:
"""Internal snapping points. So far, internal snapping points are generated by
`layer_refinement_specs` and lumped element.
Parameters
----------
structures : List[Structure]
List of physical structures.
lumped_elements : List[LumpedElementType]
List of lumped elements.
cached_corners_and_convexity : Optional[list[CachedCornersAndConvexity]]
Cached corners and convexity data.
Returns
-------
List[CoordinateOptional]
List of snapping points coordinates.
"""
# no need to generate anything if autogrid is not used
if not self.auto_grid_used:
return []
snapping_points = []

both internal_snapping points and internal_override_structures are not used. The design at that time is that LayerRefinement should only take effect for AutoGrid. QuasiUniformGrid is almost uniform, and should only be used for very basic grid setup.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 12, 2025

Diff Coverage

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

  • tidy3d/plugins/smatrix/component_modelers/terminal.py (100%)
  • tidy3d/plugins/smatrix/ports/base_lumped.py (100%)
  • tidy3d/plugins/smatrix/ports/wave.py (100%)

Summary

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

@dmarek-flex
Copy link
Contributor Author

QuasiUniformGrid is almost uniform, and should only be used for very basic grid setup.

Very well, then I think I will change validator to check for AutoGrid

Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

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

Thanks!

@dmarek-flex dmarek-flex force-pushed the FXC-4098-Fix-GridSpec-handling-in-TerminalComponentModeler branch from 2a36422 to 59172b9 Compare November 12, 2025 18:53
Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 left a comment

Choose a reason for hiding this comment

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

Nice!

@dmarek-flex dmarek-flex force-pushed the FXC-4098-Fix-GridSpec-handling-in-TerminalComponentModeler branch 2 times, most recently from fac9fbf to 7577865 Compare November 13, 2025 14:16
@dmarek-flex dmarek-flex force-pushed the FXC-4098-Fix-GridSpec-handling-in-TerminalComponentModeler branch from 7577865 to 1699eb3 Compare November 13, 2025 14:45
@dmarek-flex dmarek-flex added this pull request to the merge queue Nov 13, 2025
Merged via the queue into develop with commit bad66c6 Nov 13, 2025
26 checks passed
@dmarek-flex dmarek-flex deleted the FXC-4098-Fix-GridSpec-handling-in-TerminalComponentModeler branch November 13, 2025 15:56
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.

4 participants