Skip to content

Fixed aperture bounding boxes ignoring the internal transformation#171

Open
jacobdparker wants to merge 2 commits into
mainfrom
fix/polygonal-aperture-bounds
Open

Fixed aperture bounding boxes ignoring the internal transformation#171
jacobdparker wants to merge 2 commits into
mainfrom
fix/polygonal-aperture-bounds

Conversation

@jacobdparker

Copy link
Copy Markdown
Contributor

Summary

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), 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:

  • EllipticalAperture bounds transformed 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-counts the internal transformation once the bounds include it; it now tests against half_width directly.

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 under pytest-randomly) with a small tolerance for wire-interpolation round-off.

pytest optika/apertures: 20485 passed.

🤖 Generated with Claude Code

@codecov

codecov Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.35%. Comparing base (e9a4eb5) to head (9479771).

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           
Flag Coverage Δ
unittests 99.35% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@roytsmart

Copy link
Copy Markdown
Collaborator

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?

jacobdparker and others added 2 commits June 15, 2026 11:33
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>
@jacobdparker jacobdparker force-pushed the fix/polygonal-aperture-bounds branch from 1759e2b to 9479771 Compare June 15, 2026 17:58
@jacobdparker

Copy link
Copy Markdown
Contributor Author

Good point — switched these to a closed form rather than sampling the wire. A boundary point is c + a*e_a*cos(t) + b*e_b*sin(t), so the extent along each world axis is sqrt((a*e_a)^2 + (b*e_b)^2) (since max(A cos t + B sin t) = sqrt(A^2 + B^2)). That's exact under rotation and reduces to the (radius.x, radius.y) box when untransformed. CircularAperture is already analytic and rotation-invariant, so it's unchanged. Done in 9479771.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants