-
Notifications
You must be signed in to change notification settings - Fork 70
fix: consistency between mode source and mode frame (FXC-4332) #3169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
fix: consistency between mode source and mode frame (FXC-4332) #3169
Conversation
There was a problem hiding this 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
| 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]): |
There was a problem hiding this comment.
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.
| 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]): |
There was a problem hiding this comment.
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.
| 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.
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
|
|
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 |
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.
relax_precisionflag toGrid.discretize_indsto treat box boundaries numerically close to cell boundaries as equal; adjusts index calculation and final return castingpts_minconsistently and stores indices internally as lists before casting to tuplesrelax_precision=TrueinSimulation._pec_frame_boxwhen computingspan_indsfor mode frames/sourcestest_discretize_inds_relax_precisionto validate boundary precision handlingWritten 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_precisionparameter to theGrid.discretize_indsmethod. 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:
relax_precisionparameter (defaultFalse) toGrid.discretize_indsmethod ingrid.py_pec_frame_boxinsimulation.pyto userelax_precision=Truewhen discretizing mode sourcesIssues Found:
grid.py) that prevent the relax_precision logic from working correctly when box boundaries are close to the last cell boundaryThe 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
num_cellsinstead oflen(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.tidy3d/components/grid/grid.py- the boundary condition checks on lines 637 and 641 need to be fixed before mergingImportant Files Changed
relax_precisionparameter todiscretize_indsmethod to handle floating-point precision issues. Contains logic errors in boundary condition checks that prevent snapping to the last cell boundary._pec_frame_boxto userelax_precision=Truewhen discretizing mode source boundaries for consistency with frame calculation.test_discretize_inds_relax_precisionto 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 boundariesContext used:
dashboard- Use a tolerance-based comparison (e.g., np.isclose) for floating-point numbers instead of direct equ... (source)dashboard- Update the CHANGELOG.md file when making changes that affect user-facing functionality or fix bugs. (source)