Skip to content

Conversation

@dbochkov-flexcompute
Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute commented Jan 15, 2026

There was slight discrepancy between mode source discretization and size of mode frames. This PR makes everything more consistent.


Note

Improves grid discretization robustness and aligns mode frame/source indexing.

  • Adds relax_precision flag to Grid.discretize_inds to treat box boundaries numerically close to cell boundaries as equal; adjusts index calculation and final return casting
  • Fixes extension check to use pts_min consistently and stores indices internally as lists before casting to tuples
  • Uses relax_precision=True in Simulation._pec_frame_box when computing span_inds for mode frames/sources
  • Adds test_discretize_inds_relax_precision to validate boundary precision handling

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

Greptile Summary

This PR improves consistency between mode source discretization and mode frame calculations by introducing a relax_precision parameter to the Grid.discretize_inds method. The new parameter uses tolerance-based comparison (np.isclose) to "snap" box boundaries to cell boundaries when they differ only due to floating-point precision errors, preventing discrepancies of 1 cell in the discretization.

Key Changes:

  • Added relax_precision parameter (default False) to Grid.discretize_inds method in grid.py
  • Updated _pec_frame_box in simulation.py to use relax_precision=True when discretizing mode sources
  • Added comprehensive test coverage for the new precision-relaxing behavior

Issues Found:

  • Critical logic errors in boundary condition checks (lines 637 and 641 in grid.py) that prevent the relax_precision logic from working correctly when box boundaries are close to the last cell boundary
  • Missing CHANGELOG.md entry for this user-facing bug fix

The boundary condition bugs would cause the test assertions on lines 428-429 and 437-438 to potentially fail, as the relaxed precision might not correctly snap boundaries in edge cases involving the last cells of the grid.

Confidence Score: 2/5

  • This PR contains critical logic errors in boundary condition checks that will cause incorrect behavior in edge cases
  • The PR addresses a real consistency issue between mode source discretization and frame calculations, and includes good test coverage. However, the implementation has two critical boundary condition bugs (lines 637 and 641) that use num_cells instead of len(cell_bounds) in the comparison, preventing the relax_precision logic from working correctly when box boundaries are near the last cell boundary. These bugs would likely cause test failures and incorrect discretization in production for certain grid configurations.
  • Pay close attention to tidy3d/components/grid/grid.py - the boundary condition checks on lines 637 and 641 need to be fixed before merging

Important Files Changed

Filename Overview
tidy3d/components/grid/grid.py Added relax_precision parameter to discretize_inds method to handle floating-point precision issues. Contains logic errors in boundary condition checks that prevent snapping to the last cell boundary.
tidy3d/components/simulation.py Updated _pec_frame_box to use relax_precision=True when discretizing mode source boundaries for consistency with frame calculation.
tests/test_components/test_grid.py Added comprehensive test test_discretize_inds_relax_precision to verify floating-point precision handling in grid discretization.

Sequence Diagram

sequenceDiagram
    participant Sim as Simulation._pec_frame_box
    participant Grid as Grid.discretize_inds
    participant Relax as relax_precision logic
    
    Note over Sim: Mode source or absorber<br/>needs frame box calculation
    
    Sim->>Grid: discretize_inds(obj, relax_precision=True)
    
    Note over Grid: Calculate base indices<br/>from box boundaries
    
    Grid->>Grid: Find ind_min (largest boundary ≤ box_min)
    Grid->>Grid: Find ind_max (smallest boundary > box_max)
    
    alt extend=True
        Grid->>Grid: Adjust indices for field interpolation
    end
    
    alt relax_precision=True
        Grid->>Relax: Check each dimension
        
        loop for each dimension
            Relax->>Relax: Check if box_min ≈ cell_bounds[min_ind+1]
            alt boundaries are close (np.isclose)
                Relax->>Relax: Snap: increment min_ind
            end
            
            Relax->>Relax: Check if box_max ≈ cell_bounds[max_ind]
            alt boundaries are close (np.isclose)
                Relax->>Relax: Snap: increment max_ind
            end
        end
    end
    
    Grid-->>Sim: Return [(ind_min, ind_max), ...]
    
    Note over Sim: Use indices to construct<br/>PEC frame boundaries
Loading

Context used:

  • Rule from dashboard - Use a tolerance-based comparison (e.g., np.isclose) for floating-point numbers instead of direct equ... (source)
  • Rule from dashboard - Update the CHANGELOG.md file when making changes that affect user-facing functionality or fix bugs. (source)

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

box_max = pts_max[dim]
# Check if the cell boundary after current min is close to the box min,
# if it is close enough then choose that to be the new minimum cell index
if min_ind + 1 < num_cells and np.isclose(box_min, cell_bounds[min_ind + 1]):
Copy link

Choose a reason for hiding this comment

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

logic: condition should be min_ind + 1 < len(cell_bounds) instead of min_ind + 1 < num_cells to allow checking the last cell boundary. With num_cells = len(cell_bounds) - 1, the current condition prevents checking when min_ind = num_cells - 1, but cell_bounds[num_cells] is a valid index.

Suggested change
if min_ind + 1 < num_cells and np.isclose(box_min, cell_bounds[min_ind + 1]):
if min_ind + 1 < len(cell_bounds) and np.isclose(box_min, cell_bounds[min_ind + 1]):
Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/components/grid/grid.py
Line: 637:637

Comment:
**logic:** condition should be `min_ind + 1 < len(cell_bounds)` instead of `min_ind + 1 < num_cells` to allow checking the last cell boundary. With `num_cells = len(cell_bounds) - 1`, the current condition prevents checking when `min_ind = num_cells - 1`, but `cell_bounds[num_cells]` is a valid index.

```suggestion
                if min_ind + 1 < len(cell_bounds) and np.isclose(box_min, cell_bounds[min_ind + 1]):
```

How can I resolve this? If you propose a fix, please make it concise.

inds_list[dim][0] += 1
# Same but for the max cell boundary. If the current cell boundary is close to the box bounds,
# then it is considered equal and the stop index should be incremented
if max_ind < num_cells and np.isclose(box_max, cell_bounds[max_ind]):
Copy link

Choose a reason for hiding this comment

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

logic: condition should be max_ind < len(cell_bounds) instead of max_ind < num_cells. When max_ind = num_cells, cell_bounds[max_ind] is still a valid access (the last boundary), and we should check if box_max is close to it.

Suggested change
if max_ind < num_cells and np.isclose(box_max, cell_bounds[max_ind]):
if max_ind < len(cell_bounds) and np.isclose(box_max, cell_bounds[max_ind]):
Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/components/grid/grid.py
Line: 641:641

Comment:
**logic:** condition should be `max_ind < len(cell_bounds)` instead of `max_ind < num_cells`. When `max_ind = num_cells`, `cell_bounds[max_ind]` is still a valid access (the last boundary), and we should check if `box_max` is close to it.

```suggestion
                if max_ind < len(cell_bounds) and np.isclose(box_max, cell_bounds[max_ind]):
```

How can I resolve this? If you propose a fix, please make it concise.

@github-actions
Copy link
Contributor

Diff Coverage

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

  • tidy3d/components/grid/grid.py (100%)
  • tidy3d/components/simulation.py (100%)

Summary

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

@dmarek-flex
Copy link
Contributor

Thanks for the fix! I wonder if we should always use the relax precision option and maybe use a customizable tolerances with a default value. Any thoughts on that?

@dbochkov-flexcompute
Copy link
Contributor Author

Thanks for the fix! I wonder if we should always use the relax precision option and maybe use a customizable tolerances with a default value. Any thoughts on that?

I think as long as we apply it consistently, either way is fine. Whether to use the relax precision or not should be determined by other factors. But making sure discretization always work symmetrically in positive and negative directions would be nice eventually

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.

3 participants