Solve for the stops once when linearizing, not three times - #211
Conversation
Denormalizing a grid does two unrelated things: it solves for where the stops put the field and pupil, and it applies an affine map. The solve costs 0.13 s and depends on nothing but the wavelengths; the map is free. `linearize` denormalizes three grids on the same wavelengths, one per fit, so it solved three times for the same answer, and a fourth for `rayfunction_default`. Split the two halves apart, so that a caller with several grids can solve once: _calc_rayfunction_stops_denormalize the solve _denormalize_grid_from_rays the affine map _denormalize_grid both, as before and have `linearize` solve once, denormalize both of its grids, and pass each fit a grid which is already physical, so their own denormalization is a no-op. before: 4 stop solves, 0.59 s of 3.98 s after: 2 stop solves, 0.29 s of 3.59 s The one which remains is `rayfunction_default`'s, on the default grid rather than the one being linearized. Since the fits can no longer be handed a grid of `None`, `linearize` resolves the defaults itself, and the pupil grid `area_effective` invents when given none becomes `_pupil_vertices_default` so that both can reach it. The two fits keep falling back to `grid_input.pupil` as before: those grids differ because one wants cell vertices and the other wants sample points, and reconciling them is #187's job, not this one's. Every product of `linearize` is unchanged bit for bit: all twenty distortion coefficients, all ten vignetting coefficients, the direction, the scene and sensor coordinates the distortion is fit to, and the illumination the vignetting is fit to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #211 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 121 121
Lines 7384 7402 +18
=========================================
+ Hits 7384 7402 +18
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:
|
| y=na.linspace(-1, 1, axis="_pupil_y", num=11), | ||
| ) | ||
|
|
||
| def _calc_rayfunction_stops_denormalize( |
There was a problem hiding this comment.
Why is this function necessary?
There was a problem hiding this comment.
It isn't. Removed in 0e0f0f1.
The wrapper only existed to fill in four arguments that every caller passes identically, which was papering over a problem rather than fixing it: the defaults on _calc_rayfunction_stops were simply wrong. Its two axis parameters had no defaults at all, even though both callers pass the class constants _axis_pupil_stop and _axis_field_stop, and its sample counts defaulted to 101, which nothing in the package has ever used — every call site passes 21.
Giving it the values its callers actually want removes the wrapper and the same boilerplate from rayfunction_stops, which was repeating all four arguments too:
return self._calc_rayfunction_stops(self.grid_input.wavelength)Net 27 lines fewer than the previous commit, and one fewer way to get a stop rayfunction rather than one more.
Still bit-identical to main across all 37 deterministic products of linearize, and still 2 stop solves instead of 4.
`_calc_rayfunction_stops_denormalize` existed only to fill in four arguments which every caller passes identically. It is not needed: the defaults on `_calc_rayfunction_stops` were simply wrong. Its axes had no defaults at all even though both callers pass the class constants, and its sample counts defaulted to 101, which nothing has ever used. Give it the values its callers actually want, and the wrapper and the boilerplate in `rayfunction_stops` both go away. Net 27 lines fewer, and the products of `linearize` remain identical to main across all 37 arrays.
1 participant
_denormalize_griddoes two unrelated things: it solves for where the stops put the field and pupil, and it applies an affine map. The solve costs 0.13 s and depends on nothing but the wavelengths. The map is free.linearizedenormalizes three grids on the same wavelengths, one per fit, so it solved three times for the same answer, plus a fourth forrayfunction_default:There is a cache for this,
rayfunction_stops, and it works. But it is keyed ongrid_input.wavelength, so the moment a caller supplies its own wavelengths it never hits. Nor was the cost amortised across calls: a secondlinearizeon the same wavelengths ran all four again.What this does
Splits the two halves apart so a caller with several grids can solve once:
_calc_rayfunction_stops_denormalize_denormalize_grid_from_rays_denormalize_gridlinearizethen solves once, denormalizes both of its grids, and passes each fit a grid that is already physical, so their own_denormalize_gridhits its early return and never solves.The remaining one is
rayfunction_default's, which is on the default grid rather than the one being linearized. That is a separate question about wheredirectionshould come from.Nothing changes
Every deterministic product of
linearize, compared againstmainin a worktree onesis.flights.f1.optics.design_single()over nine wavelengths. All 37 arrays equal bit for bit:This is exact rather than approximate because the two stop solves it replaces were already producing identical results, which I verified directly, and because
linearizekeeps the existing order of operations. In particular it still takes pupil cell centers from the normalized vertices, before denormalizing, as it does today. Denormalizing first would be equivalent in exact arithmetic, since an affine map commutes with an average, but differs by an ulp in floating point:Not worth giving up a provable claim for.
One wrinkle
The fits can no longer be handed a grid of
None, solinearizeresolves the defaults itself. The pupil gridarea_effectiveinvents when given none moves out of the method body into_pupil_vertices_default, so both can reach it.The two fits keep falling back to
grid_input.pupilexactly as before. Those two defaults differ becausearea_effectiveneeds cell vertices, to weight each ray by the area of its pupil cell, while the fits need sample points. Reconciling them is #187's job, and doing it here would silently change results in a PR that is otherwise provably inert.(Edited: this paragraph previously claimed #187 would also fix a knife-edge sampling error making
vignetting's illumination 15% low. That was wrong — I measured a grid the code does not use.grid_inputis built withcenters=True, so its samples are already interior, and the real effect is rms 1.9% on the normalized illumination. Retracted in detail at #208 (comment).)Tests
Full suite: 17774 passed, 304 skipped, 17 xfailed.
_sequential.pyat 100% line coverage.🤖 Generated with Claude Code