bug fix: spatially varying plane sources and PMC walls under mirror symme… - #430
Closed
bruxillensis wants to merge 1 commit into
Closed
bug fix: spatially varying plane sources and PMC walls under mirror symme…#430bruxillensis wants to merge 1 commit into
bruxillensis wants to merge 1 commit into
Conversation
…try (ymahlau#425) Four independent defects made a symmetry-reduced simulation wrong for any source with spatial variation along a symmetry axis. All were invisible to the existing symmetry tests, which only use UniformPlaneSource with normalize_by_energy=False and direction="+" -- a combination blind to every one of them. 1. Profile center re-centered on the reduced quadrant. TFSFPlaneSource._get_center derived the transverse center from the object's clipped extent, so a Gaussian spot moved to the middle of the kept half instead of staying on the symmetry plane. The reduction discarded the pre-clip extent, so the source could not know it had been cut; reduce_resolved_slices now reports the per-axis low-side clip and place_objects records it as SimulationObject._symmetry_clip_low. 2. Normalization summed over the clipped plane. normalize_by_energy and the Gaussian profile's own sum normalization both divided by a quarter-plane sum, inflating the injected amplitude by 2**(n_axes/2) -- exactly 2x for two symmetric axes. 3. Profile mirrored about `center` along the vertical axis. v_basis = cross(wave_vector, u_basis) has a sign that depends on the propagation axis and direction, so the sampled profile was reflected for e.g. direction="-" along z (and, oppositely, direction="+" along y). Predates the symmetry feature by ~14 months and is invisible for any profile symmetric about its own center; it also blocked fix 1, since a correctly off-center profile got flipped out of the array. 4. The PMC symmetry wall over-constrained the field. Along a symmetry axis the six components split by Yee offset: tangential E and normal H sit at offset 0, normal E and tangential H at offset 1/2. The mirror plane lands on the reduced domain's low cell edge, so the tangential-H node a magnetic mirror must zero is at local -1 -- outside the array, where zero-padding already supplies it. Inserting a PerfectMagneticConductor instead zeroed tangential H at local 0, a full cell inside the domain. PEC still needs its wall (tangential E has no node on the plane). make_symmetry_walls now builds walls only for electric (-1) planes. Validation. A symmetry-reduced run is compared cell-for-cell against the half of an equivalent full-domain run that it is supposed to represent (no unfolding involved). The figure quoted is the maximum absolute deviation of E over that half, divided by the peak |E| of the full-domain run -- so 0.0019 means the worst cell in the reduced domain is off by 0.19% of the full-domain field peak. Source is a Gaussian beam except where a dipole group is named. Values are before -> after this change: PEC on x/y/z (single axis) 0.0019 -> 0.0019 (unchanged) PMC on x/y/z (single axis) 0.2768 -> 0.0019 PEC+PMC pairs (4 orientations) 0.2753 -> 0.0019 PMC on x/y (mirror dipole pair) 0.6819 -> 0.0002 PMC+PMC (mirror dipole quad) 1.8677 -> 0.0003 PEC-only axes are untouched by this commit; they are listed for contrast, since they were already correct and are what made the PMC error stand out. The residual ~0.2% on the beam cases is dominated by PML reflection and beam truncation, not by the symmetry plane -- the dipole cases, which have neither, land at 0.02-0.03%. The PMC fix converges under grid refinement -- 0.00142, 0.00053, 0.00032 at 200, 100 and 50 nm spacing -- so it is a consistent discretization rather than a cancellation that happens to work at one resolution. With all four applied the injected reduced profile equals the kept half of the full-domain profile to float32 roundoff (~1e-6 relative). Known remaining limitation, not addressed here: unfold_fields applies one array flip to all three components, but the correct reconstruction map differs by Yee staggering class. Measured over the PML-free interior, three components per axis reconstruct to ~0.1% of the full-domain peak and the other three to 2-12%, and which class is exact flips between axes. The reduced simulation itself is correct; only the unfolded reconstruction is affected.
|
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 (8)
📝 WalkthroughWalkthroughSymmetry reduction now records low-side clipping, propagates it to placed objects, adjusts plane-source positioning and normalization, and creates explicit walls only for PEC symmetry. New tests cover profile orientation, Gaussian-field equivalence, amplitude scaling, and wall generation. ChangesSymmetry-aware source reduction
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SimulationConfig
participant place_objects
participant reduce_resolved_slices
participant PlaneSource
SimulationConfig->>place_objects: configure symmetry and objects
place_objects->>reduce_resolved_slices: reduce object slices
reduce_resolved_slices-->>place_objects: reduced slices and clip_low
place_objects->>PlaneSource: attach _symmetry_clip_low
PlaneSource->>PlaneSource: adjust center and normalization
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #430 +/- ##
==========================================
+ Coverage 90.52% 90.54% +0.01%
==========================================
Files 92 92
Lines 11920 11943 +23
Branches 1820 1823 +3
==========================================
+ Hits 10791 10814 +23
Misses 783 783
Partials 346 346 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Owner
|
@bruxillensis, my AI agent also did a fix over night, see #431 :D I guess we duplicated work here a little. I would tend towards going forward with my PR because it also fixes ModeSource and works with non-square profiles |
Merged
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
2 participants
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 for #425. I hope its alright I tackled this @ymahlau, I felt responsible :).
Four independent bugs made a symmetry-reduced simulation wrong for any source with spatial variation along a symmetry axis. All were invisible to the existing symmetry tests, which only use UniformPlaneSource with normalize_by_energy=False and direction="+" -- a combination blind to every one of them.
Profile center re-centered on the reduced quadrant. TFSFPlaneSource._get_center derived the transverse center from the object's clipped extent, so a Gaussian spot moved to the middle of the kept half instead of staying on the symmetry plane. The reduction discarded the pre-clip extent, so the source could not know it had been cut; reduce_resolved_slices now reports the per-axis low-side clip and place_objects records it as SimulationObject._symmetry_clip_low.
Normalization summed over the clipped plane. normalize_by_energy and the Gaussian profile's own sum normalization both divided by a quarter-plane sum, inflating the injected amplitude by 2**(n_axes/2) -- exactly 2x for two symmetric axes.
Profile mirrored about
centeralong the vertical axis. v_basis = cross(wave_vector, u_basis) has a sign that depends on the propagation axis and direction, so the sampled profile was reflected for e.g. direction="-" along z (and, oppositely, direction="+" along y). Predates the symmetry feature by ~14 months and is invisible for any profile symmetric about its own center; it also blocked fix 1, since a correctly off-center profile got flipped out of the array.The PMC symmetry wall over-constrained the field. Along a symmetry axis the six components split by Yee offset: tangential E and normal H sit at offset 0, normal E and tangential H at offset 1/2. The mirror plane lands on the reduced domain's low cell edge, so the tangential-H node a magnetic mirror must zero is at local -1 -- outside the array, where zero-padding already supplies it. Inserting a PerfectMagneticConductor instead zeroed tangential H at local 0, a full cell inside the domain. PEC still needs its wall (tangential E has no node on the plane). make_symmetry_walls now builds walls only for electric (-1) planes.
Validation. A symmetry-reduced run is compared cell-for-cell against the half of an equivalent full-domain run that it is supposed to represent (no unfolding involved). The figure quoted is the maximum absolute deviation of E over that half, divided by the peak |E| of the full-domain run -- so 0.0019 means the worst cell in the reduced domain is off by 0.19% of the full-domain field peak. Source is a Gaussian beam except where a dipole group is named. Values are before -> after this change:
PEC on x/y/z (single axis) 0.0019 -> 0.0019 (unchanged)
PMC on x/y/z (single axis) 0.2768 -> 0.0019
PEC+PMC pairs (4 orientations) 0.2753 -> 0.0019
PMC on x/y (mirror dipole pair) 0.6819 -> 0.0002
PMC+PMC (mirror dipole quad) 1.8677 -> 0.0003
PEC-only axes are untouched by this commit; they are listed for contrast, since they were already correct and are what made the PMC error stand out. The residual ~0.2% on the beam cases is dominated by PML reflection and beam truncation, not by the symmetry plane -- the dipole cases, which have neither, land at 0.02-0.03%.
The PMC fix converges under grid refinement -- 0.00142, 0.00053, 0.00032 at 200, 100 and 50 nm spacing -- so it is a consistent discretization rather than a cancellation that happens to work at one resolution.
With all four applied the injected reduced profile equals the kept half of the full-domain profile to float32 roundoff (~1e-6 relative).
Known remaining limitation, not addressed here: unfold_fields applies one array flip to all three components, but the correct reconstruction map differs by Yee staggering class. Measured over the PML-free interior, three components per axis reconstruct to ~0.1% of the full-domain peak and the other three to 2-12%, and which class is exact flips between axes. The reduced simulation itself is correct; only the unfolded reconstruction is affected.
Found issue, not addressed here: GaussianPlaneSource profile is transposed for propagation along y. GaussianPlaneSource._get_amplitude_raw builds its profile as (horizontal_axis, vertical_axis) and then inserts the singleton propagation axis. For propagation along y, get_oriented_transverse_axes(1) returns (2, 0) — horizontal = z, vertical = x — which is the only axis ordering where horizontal > vertical. The resulting array is therefore (z, 1, x) where the field slice expects (x, 1, z).
Consequences:
Summary by CodeRabbit
Bug Fixes
Tests