Conversation
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
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.
Fixes NanoComp/meep#3311.
Symptom
The example in NanoComp/meep#3311 builds a
mp.Prismwith a 200 × 4 rectangular base in the xy plane,height=2.0,axis=ẑ,sidewall_angle=30°. The yz cross section rendered as a six-side polygon instead of the expected four-sided trapezoid.Root cause
utils/geom.c:3139, inintersect_line_with_segment— the test for "does the query pointq0lie on the edgeq1q2?":It judged collinearity from the cosine of the angle subtended at
q0. For a point at perpendicular distancedfrom the midpoint of an edge of lengthL, that cosine is≈ 1 - 8d²/L², so the test accepted everything withAn angular tolerance used as a distance test — so the effective slop scales with edge length.
node_in_or_on_polygon(geom.c:3204) treats that verdict as "on the boundary" and returnsinclude_boundaries = 1, givingpoint_in_prisma false-positive halo around the entire region.For this geometry
L = 200→ halo = 0.2236. Measured against libctl by bisection, matching the closed form to 7 digits:true + haloConfirmed across aspect ratios (edge 200/40/8/4 → 0.22361/0.04472/0.00894/0.00447) and independent of the sidewall angle — identical at 0°, 5°, 30°, 45°, -30°, — and of height.
Why the sidewall angle exposes it, and why six sides
Meep only attributes a point to an object whose bounding box contains it.
get_prism_bounding_boxcorrectly unions the bottom and top polygons, giving y ∈ [-2, 2], so the rendered profile ismin(2 - z·tan30° + halo(z), 2):2.2236 — 0.5786·zbottom + 2 vertical + 2 slanted + top = 6 sides. At
sidewall_angle=0the bounding box clips the halo uniformly and the bug is invisible — the taper doesn't cause it, it just stops the bounding box from hiding it.The fix
Measure the actual perpendicular distance instead:
1e-10 matches the convention already used one line above for the near-parallel test on
DetM. Boundary inclusion is perserved — a point exactly on an edge givescross == 0.Test
test_slanted_prism_cross_section()inutils/test-prism.c, run on a 200x4 and a 4x4 base — 45 assertions each. The 20 point probes are the discriminating ones: at five heights, a point 1e-3 inside the analytic wallw(z) = 2 - z·tan30°must be inside and one 1e-3 outside must be outside. 10/45 fail per geometry before the fix, 0 after.Two existing octagon test points needed correcting (
127.697→127.6965,101.824→101.8242): they are 6-digit-rounded edge midpoints sitting ~1.5e-4 outside the polygon, and only read as on-boundary because of the old slop. Items 6 and 7 already used full precision for the same construction.