-
Notifications
You must be signed in to change notification settings - Fork 70
fix: 2d boundaries validator was running after grid is computed #3272
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
Conversation
|
I'll have a look, just quick comment this might be related also to the motivation behind #3265 |
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.
Pull request overview
Fixes a validator-ordering regression from the Pydantic v2 migration where the “zero-size dimension boundary” validator (which can rewrite absorbing boundaries to Periodic) was running after validators that compute/cache Simulation.grid, leaving the cached grid inconsistent.
Changes:
- Move
_boundaries_for_zero_dims = validate_boundaries_for_zero_dims()earlier inSimulationso it runs before validators that can trigger grid computation/caching. - Remove the duplicate/later placement of the same validator to avoid late execution.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Must run before validators that trigger grid computation | ||
| _boundaries_for_zero_dims = validate_boundaries_for_zero_dims() | ||
|
|
Copilot
AI
Feb 11, 2026
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.
This fix relies on Pydantic validator execution order (i.e., that _boundaries_for_zero_dims runs before any validator that touches self.grid). Since validate_boundaries_for_zero_dims() mutates boundary_spec (via object.__setattr__) and grid/_grid_and_snapping_lines are cached_property, a future validator that accesses self.grid earlier could reintroduce the stale-cache bug. Consider making the zero-dim boundary normalization a model_validator(mode="before") (operating on the input dict) or explicitly clearing any cached properties that depend on boundary_spec when it is changed, so correctness doesn’t depend on declaration order.
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.
Probably using mode="before" makes sense for such validators @yaugenst-flex what do you think?
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.
That's right, that wouldve been also my preferred choice but there are some that make this quite tricky. There are a handfull of object.__setattr__ instances currently, and we should try to avoid adding more 😬
| # Must run before validators that trigger grid computation | ||
| _boundaries_for_zero_dims = validate_boundaries_for_zero_dims() | ||
|
|
Copilot
AI
Feb 11, 2026
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.
Please add a regression test covering the original failure mode: a 2D simulation (one size component = 0) with an absorbing/PML boundary on that axis should have its boundary coerced to Periodic before any grid computation, and the resulting cached sim.grid should reflect the corrected boundary (e.g., no extra PML cells/layers along the zero-size axis). Current tests assert the warning, but don’t appear to assert that the computed grid/cell counts are unaffected by the initial absorbing boundary.
Ah, yeah, that one is nice! |
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
|
yaugenst-flex
left a comment
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.
So I just tested it and #3265 actually fixes this bug, so I think we can just close this PR?
Incidentally, this branch actually does not fully resolve the bug:
import tidy3d as td
src = td.PlaneWave(
source_time=td.GaussianPulse(freq0=2.5e14, fwidth=1e13),
center=(0,0,0), size=(td.inf,0,td.inf), direction='+', pol_angle=0.0
)
common = dict(size=(1,1,0), run_time=1e-12, sources=[src], grid_spec=td.GridSpec.uniform(dl=0.1))
sim_periodic = td.Simulation(
**common,
boundary_spec=td.BoundarySpec(x=td.Boundary.periodic(), y=td.Boundary.periodic(), z=td.Boundary.periodic()),
)
sim_pml = td.Simulation(
**common,
boundary_spec=td.BoundarySpec(x=td.Boundary.periodic(), y=td.Boundary.periodic(), z=td.Boundary.pml()),
)
print("periodic", sim_periodic.grid.num_cells)
print("pml->periodic", sim_pml.grid.num_cells)
print("equal", sim_periodic.grid.num_cells == sim_pml.grid.num_cells)gives:
periodic [10, 10, 1]
pml->periodic [10, 10, 25]
equal False
Ahh that's funny, the initial modification that does fix this had a bit more (some updates in But yeah Marco's PR does seem to fix the failing backend test so I'll close this one. |
So a sneaky bug was introduced by the pydantic v2 migration and was causing one backend test to fail.
The 2d boundary validator has a side-effect: if the simulation has 0 size and absorbing boundaries along some dimension, it overwrites those boundaries to Periodic and issues a warning.
After the refactor, this was running only after the grid was computed by some other validators, and cached - causing it to be stored with the wrong number of grid cells (the pml boundaries were added).
The fix to move the validator up works, but this seems kind of flaky. I think in general we try to avoid such types of validators, but this was done to avoid a user annoyance where it was not straightforward to set a default 2D simulation (had to touch the boundary spec, which in many cases users don't have to touch).
What are we thinking going forward?
Note
Low Risk
Small, localized change to Pydantic validator ordering; behavior only changes for zero-size dimensions where boundaries are auto-normalized, but it can affect grid sizing/caching in those cases.
Overview
Fixes validator ordering in
Simulationsovalidate_boundaries_for_zero_dims()runs before any validators that may compute/cache the grid.This prevents 2D/zero-thickness simulations with absorbing/PML boundaries from computing grid cells using the wrong (pre-normalized) boundary spec, avoiding incorrect cached grids and related backend test failures.
Written by Cursor Bugbot for commit 9cd5bc2. This will update automatically on new commits. Configure here.