Fixed aperture bounding boxes ignoring the internal transformation#171
Fixed aperture bounding boxes ignoring the internal transformation#171jacobdparker wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #171 +/- ##
=======================================
Coverage 99.34% 99.35%
=======================================
Files 116 116
Lines 5974 6024 +50
=======================================
+ Hits 5935 5985 +50
Misses 39 39
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
My concern is that if the aperture is circular/elliptical, taking the min/max of the wore might not capture the full extent. Can we handle these cases analytically? |
AbstractPolygonalAperture.bound_lower/bound_upper were computed from the raw vertices, while __call__ and wire() apply the aperture's internal transformation. For apertures with a nonzero internal transformation (e.g. a decentered grating aperture), any consumer of the bounding box (stratified wavefield sampling, sensor pixel grids, pupil normalization) saw the wrong region of the surface. EllipticalAperture had the related defect of transforming only the bounding-box corner points, which is incorrect under rotation; its bounds are now derived from the wire, matching CircularSectorAperture. RectangularAperture.__call__ expressed the rectangle in terms of bound_lower/bound_upper while also inverse-transforming the position, which double-counted the internal transformation once the bounds included it; it now tests against half_width directly. The generic bound tests now assert that the bounds enclose the wire and that the wire centroid is accepted by active, non-inverted apertures (nominal values, with a small tolerance for interpolation round-off, since uncertain parameters may be redrawn between evaluations). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address review feedback: deriving EllipticalAperture.bound_lower/upper from wire().min()/max() sampled the boundary at a finite number of points, so the true extent between samples was underestimated whenever the internal transformation rotated the ellipse off the coordinate axes. Replace the wire sampling with a closed form. A boundary point is p(t) = c + a*e_a*cos(t) + b*e_b*sin(t), where c is the transformed center and e_a, e_b are the transformed local axes, so the extent along any world component is sqrt((a*e_a)^2 + (b*e_b)^2). This is exact under rotation and reduces to the axis-aligned (radius.x, radius.y) box when untransformed. CircularAperture bounds are already analytic and rotation-invariant, so they are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1759e2b to
9479771
Compare
|
Good point — switched these to a closed form rather than sampling the wire. A boundary point is |
Summary
AbstractPolygonalAperture.bound_lower/bound_upperwere computed from the raw vertices, while__call__andwire()apply the aperture's internaltransformation. For apertures with a nonzero internal transformation (e.g. a decentered grating aperture), every consumer of the bounding box (stratified sampling grids, sensor pixel grids, pupil normalization) saw the wrong region of the surface.Found while validating a physical-optics model of ESIS f1: the grating's trapezoidal aperture is decentered by
-12.26 mm, so the sampling grid only overlapped a ~4.7 mm sliver of the real 16.9 mm aperture, broadening the Huygens PSF by 4x in the dispersion direction.Two related defects fixed in the same commit:
EllipticalAperturebounds transformed only the bounding-box corner points, which is incorrect under rotation; its bounds are now derived from the wire, matchingCircularSectorAperture.RectangularAperture.__call__expressed the rectangle in terms ofbound_lower/bound_upperwhile also inverse-transforming the position, which double-counts the internal transformation once the bounds include it; it now tests againsthalf_widthdirectly.Tests
The generic bound tests now assert that the bounds enclose the wire, and that the wire centroid is accepted by active, non-inverted apertures; plus a targeted regression test for a decentered
RectangularAperture. Comparisons use nominal values (uncertain parameters may be redrawn between independent evaluations underpytest-randomly) with a small tolerance for wire-interpolation round-off.pytest optika/apertures: 20485 passed.🤖 Generated with Claude Code