fixed symmetry bug - #431
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughSymmetry handling now distinguishes electric and magnetic planes across reduction, parity mapping, field padding, unfolding, mode solving, and source placement. New validations reject unsupported placements, while expanded tests cover reduced fields, detectors, modes, sources, and mirror index behavior. ChangesSymmetry-aware simulation pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SimulationConfig
participant place_objects
participant SymmetryReduction
participant FDTDUpdates
participant DetectorUnfolding
SimulationConfig->>place_objects: provide symmetry planes
place_objects->>SymmetryReduction: reduce slices and create electric walls
SymmetryReduction->>FDTDUpdates: configure reduced grid and halo behavior
FDTDUpdates->>DetectorUnfolding: produce reduced detector states
DetectorUnfolding-->>SimulationConfig: reconstruct full-domain outputs
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #431 +/- ##
==========================================
+ Coverage 90.52% 90.63% +0.10%
==========================================
Files 92 93 +1
Lines 11920 12166 +246
Branches 1820 1864 +44
==========================================
+ Hits 10791 11027 +236
- Misses 783 787 +4
- Partials 346 352 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fdtdx/core/physics/modes.py`:
- Around line 399-400: Add an explicit validation guard in the mode-solving path
around the bend parameters, including the path reached by
ModeOverlapDetector._compute_mode_fields: reject any configuration where both
bend_radius/bend_axis and a mirrored axis are set, before forwarding parameters
to the full-cross-section solve. Raise the established invalid-configuration
exception with a clear message, while preserving existing behavior for bends
without mirroring and mirroring without bends.
In `@src/fdtdx/core/physics/symmetry.py`:
- Around line 245-247: The mode-symmetry residual must remain a JAX array during
traced execution. Update project_onto_parity to return residual without
converting it via float, and revise compute_mode_symmetry_reduced and its
callers so the residual threshold check and diagnostic logging occur outside
jax.jit-traced code; otherwise explicitly prevent tracing of
ModePlaneSource.apply_params.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a7962834-122b-4b9e-a9bd-c7e075131318
📒 Files selected for processing (21)
src/fdtdx/core/physics/modes.pysrc/fdtdx/core/physics/symmetry.pysrc/fdtdx/fdtd/initialization.pysrc/fdtdx/fdtd/symmetry.pysrc/fdtdx/fdtd/update.pysrc/fdtdx/objects/boundaries/boundary.pysrc/fdtdx/objects/boundaries/pmc.pysrc/fdtdx/objects/detectors/diffractive.pysrc/fdtdx/objects/detectors/mode.pysrc/fdtdx/objects/object.pysrc/fdtdx/objects/sources/dipole.pysrc/fdtdx/objects/sources/linear_polarization.pysrc/fdtdx/objects/sources/mode.pysrc/fdtdx/objects/sources/tfsf.pysrc/fdtdx/objects/sources/tfsf_region.pytests/integration/fdtd/test_symmetry_mode_source.pytests/integration/fdtd/test_symmetry_reduction.pytests/simulation/physics/boundaries/test_symmetry_structured_field.pytests/unit/core/physics/test_symmetry.pytests/unit/fdtd/test_symmetry_unfold.pytests/unit/objects/sources/test_symmetry_sources.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/fdtdx/core/physics/symmetry.py (1)
270-283: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftResidual concretization under
jax.jitstill unresolved.
project_onto_parityis called fromcompute_mode_symmetry_reduced, which is invoked fromModePlaneSource.apply()/ModeOverlapDetector.apply().float(residual)at line 282 forces a JAX value to a Python scalar; if theseapply()paths are ever traced underjax.jit(e.g. a jitted optimization loop callingapply_params), this raisesConcretizationTypeError. This is the same code flagged in a prior review round and remains unchanged.🐛 Suggested direction (unchanged from prior review)
- residual = jnp.where(norm > 0, jnp.linalg.norm(jnp.abs(field - projected)) / norm, 0.0) - return projected, float(residual) + residual = jnp.where(norm > 0, jnp.linalg.norm(jnp.abs(field - projected)) / norm, 0.0) + return projected, residualThen move the
residual > 0.9/residual > 0.3branches incompute_mode_symmetry_reducedout of any traced call path (or document/enforce thatModePlaneSource.apply_paramsmust not be traced).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fdtdx/core/physics/symmetry.py` around lines 270 - 283, Remove the Python scalar conversion from project_onto_parity by returning the JAX residual value directly, and update compute_mode_symmetry_reduced to keep residual-threshold branching outside traced execution. Ensure ModePlaneSource.apply and ModeOverlapDetector.apply remain JIT-compatible, or explicitly enforce that apply_params cannot be traced if the branching must stay in Python.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/fdtdx/core/physics/symmetry.py`:
- Around line 270-283: Remove the Python scalar conversion from
project_onto_parity by returning the JAX residual value directly, and update
compute_mode_symmetry_reduced to keep residual-threshold branching outside
traced execution. Ensure ModePlaneSource.apply and ModeOverlapDetector.apply
remain JIT-compatible, or explicitly enforce that apply_params cannot be traced
if the branching must stay in Python.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4aa8538e-546e-4b7b-ac08-3ad3a8334fc7
📒 Files selected for processing (16)
.claude/skills/fdtdx/SKILL.mdsrc/fdtdx/config.pysrc/fdtdx/core/physics/modes.pysrc/fdtdx/core/physics/symmetry.pysrc/fdtdx/fdtd/initialization.pysrc/fdtdx/fdtd/symmetry.pysrc/fdtdx/fdtd/update.pysrc/fdtdx/objects/boundaries/boundary.pysrc/fdtdx/objects/boundaries/pmc.pytests/integration/fdtd/test_symmetry_mode_source.pytests/integration/fdtd/test_symmetry_reduction.pytests/simulation/physics/boundaries/test_symmetry_structured_field.pytests/unit/core/physics/test_symmetry.pytests/unit/fdtd/test_symmetry_unfold.pytests/unit/fdtd/test_update.pytests/unit/objects/sources/test_symmetry_sources.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/fdtdx/fdtd/initialization.py
- src/fdtdx/objects/boundaries/boundary.py
- tests/unit/objects/sources/test_symmetry_sources.py
- src/fdtdx/core/physics/modes.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: spatially-varying sources under
config.symmetry(closes #425)A
GaussianPlaneSourcespanning a symmetry plane was re-centred on the surviving quadrant insteadof staying centred on the plane. Tracking that down turned up a cluster of related defects in the
same area, all invisible to the existing tests because every symmetry test used a transversely
uniform source — a field with no transverse derivatives cannot detect a misplaced transverse
profile, a mirrored profile, or a mishandled cell row at the symmetry plane.
The reported bug
TFSFPlaneSource._get_centerderived the profile centre fromself.grid_shape, which is thepost-reduction shape. The geometry was clipped correctly, but the contents were regenerated from
the clipped extent, so the beam was rebuilt in the middle of the quadrant (peak at index
(14,14)of a 30×30 reduced plane instead of
(0,0)). Moving the source made no difference, since theprofile was always recentred on whatever survived clipping.
Objects now remember their pre-clip extent in reduced coordinates
(
SimulationObject.unreduced_grid_slice_tuple, with a negative start on axes where the objectcrossed a plane), and the profile centre is derived from that —
-0.5for a plane-symmetric source,i.e. half a cell below the reduced min edge.
straddles_symmetry_plane()also replaces the previousslice.start == 0heuristic, which could not distinguish "was clipped by the plane" from "happensto begin at the plane".
Adjacent defects found and fixed
direction="-"mirrored the transverse profile. The in-planevbasis came fromcross(wave_vector, u_basis), so a backward-propagating source flipped its profile about thecentre. Invisible for a centred radially symmetric spot; wrong for everything else, and it made
the centre fix alone zero out the injected field.
descending there (
(2, 0)). On a non-square plane it crashed outright:TypeError: div got incompatible shapes for broadcasting: (3,8,1,4), (1,4,1,8).2^k(normalize_by_energy=False) or2^(k/2)(default),because
profile.sum()and the energy sum ran over the clipped plane. Both now normalize over thefull-domain extent, so a reduced run injects exactly what the unreduced one does.
ModePlaneSource/ModeOverlapDetectorsolved a different mode. Asking the mode solver for asymmetric solve on the reduced cross-section is inconsistent with how FDTDX rasterizes materials:
it writes one cell-centred ε array per component while the solver samples on its staggered grid.
Verified directly against tidy3d — with properly Yee-staggered ε its symmetric solve reproduces
the full spectrum to 1e-15; with FDTDX's co-located arrays it is off by ~1e-1.
nefferror for a400×200 nm Si waveguide was −0.232 / −0.108 / −0.052 at 50/25/12.5 nm (first order in Δ, so it
does not converge away). Both now mirror the reduced cross-section back to the full one, solve
there, project onto the wall's parity subspace and restrict — reproducing the mode the unreduced
simulation would use, to ≤ 2.4e-7 at every resolution. The projection also detects a wall type
that cannot support the selected mode and raises instead of injecting noise. The mode-solver
symmetryfield is no longer auto-derived; it remains for hand-built half domains.first cell — half a cell off the plane, where the odd symmetry relates it to its mirror image
rather than making it vanish. Its condition now lives in the mirrored padding halo the E-update
curl reads. Interior error for a diffracting beam went from 16% to a first-order residue
(6.7% / 3.3% / 1.6% at 50/25/12.5 nm).
half-step average against a zero halo, so a detector on the symmetry plane read
0.36093wherethe full domain has
0.72185. The same mirror halo fixes it; PEC reduction is now exact to ~1e-5across the whole plane.
unfold_*used a plain flip for every component. Components sampled on the plane(tangential E, normal H — and all co-located detector output along x and y) pair as
m±j, so theplane row was duplicated and the mirrored half shifted by one cell. Per-component index maps now
apply.
New errors and diagnostics
Placements a mirror plane cannot represent now fail with an explanation instead of silently
degrading: a point dipole on a plane (the plane is a cell edge, so the reduced run would model the
dipole plus its mirror half a cell away), a TFSF box reaching a plane (its faces in the discarded
half would be dropped, leaving an open surface), a
DiffractiveDetectorcrossing a plane (its orderbasis is set by the halved period), a plane source on a plane normal to its own propagation axis,
and a tilted or randomly displaced source crossing a plane.
Separately, a warning now names the fix when the polarization does not match the wall type. This is
worth calling out: the script in #425 uses
symmetry=(+1,-1,0)withE ∥ x, which needs(-1,+1,0)— PEC where E is normal, PMC where it is tangential. Nothing flagged it, so the reducedrun faithfully simulated a field odd about both planes.
Conventions (unchanged behaviour, now enforced and tested)
Plane-source amplitudes are chosen so a reduced run reproduces the full-domain run. Mode sources and
mode-overlap references keep carrying unit power through the plane they occupy, so
α = 1stillmeans a perfectly transmitted mode and S-parameters are unaffected; their fields are therefore
√(2^k)larger than the restriction of a full-domain unit-power mode.Tests
tests/unit/objects/sources/test_symmetry_sources.py— 40 tests: reduced vs full-domain injectionfor PEC and PMC, one and two axes, all three propagation axes, both directions, plus amplitude,
orientation, and the new errors/warning. No FDTD stepping needed, so tolerances are
1e-6ratherthan physics-level. 33 of these fail on the unfixed source.
tests/unit/core/physics/test_symmetry.py— 16 tests for the parity table, the two mirror indexmaps and the parity projection.
tests/integration/fdtd/test_symmetry_mode_source.py— 12 tests:neffand modal fidelityagainst the full domain, flux convention, source/detector consistency, incompatible-wall error.
tests/simulation/physics/boundaries/test_symmetry_structured_field.py— a diffracting beam,which is what the existing uniform-plane-wave tests could not see, comparing both the reduced
fields and the unfolded output.
Full suite green (2766 unit/integration/docs, 65 simulation including time-reversal and gradients,
plus the existing hand-built PEC/PMC quarter-domain tests),
tyandpre-commitclean.Known limitations, documented in the code
The residual PMC discrepancy is source rasterization, not the wall: profiles are applied per cell,
so a component sampled on a symmetry plane has its footprint centred half a cell off it in the
full-domain reference — the reduced run is the more symmetric of the two. Fixing that needs
per-component profile resampling, which changes every simulation, not just symmetric ones. A
user-placed (non-symmetry) PMC boundary also keeps its previous behaviour, so hand-built half
domains are untouched; the same half-cell argument applies there.
Summary by CodeRabbit