Include normalization constant in calculation of Zernike derivative#663
Include normalization constant in calculation of Zernike derivative#663crnh wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes incorrect surface-normal computation for Zernike polynomial geometries by ensuring derivatives include the same normalization constants used in the sag/polynomial evaluation, improving ray-tracing correctness (as reported in #662).
Changes:
- Multiply Zernike derivative contributions by the per-term normalization constant when computing
_surface_normal. - Update expected surface-normal reference data in geometry tests (and add explicit
nollcoverage in the reference dict).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
optiland/geometries/zernike.py |
Applies Zernike normalization constants to derivative terms used in surface-normal computation. |
tests/test_geometries.py |
Updates stored reference normals to match the corrected normalization behavior. |
| norm_constant = self.zernike._norm_constant(n, m) | ||
|
|
||
| # Partial derivatives w.r.t. x and y | ||
| dzdx = dzdx + c * (dZdrho * drho_dx + dZdphi * dphi_dx) | ||
| dzdy = dzdy + c * (dZdrho * drho_dy + dZdphi * dphi_dy) | ||
| dzdx = dzdx + norm_constant * c * (dZdrho * drho_dx + dZdphi * dphi_dx) | ||
| dzdy = dzdy + norm_constant * c * (dZdrho * drho_dy + dZdphi * dphi_dy) |
There was a problem hiding this comment.
This makes sense because get_derivative() only calculates partial derivatives; _surface_normal is the most sensible place for multiplication with the normalization constant. See also https://www.sciencedirect.com/science/article/pii/S0143816613002285.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Thanks for submitting this, @crnh.
Yes, this is the right move to assure robustness in surface normal calculations. I am okay with adding these tests to this PR, but it can also be a separate PR if you prefer. I leave that up to you. PR looks good. Feel free to merge when ready (once @drpaprika gets a chance to review as well). -Kramer |
Fix Zernike surface normal at center
As reported in #662, the normal vector calculation for Zernike coefficients currently calculates the gradient over non-normalized Zernike polynomials, resulting in incorrect normal vectors and thus incorrect ray tracing results. I fixed this by multiplying the gradient terms with the corresponding normalization constants.
When running the sample code from #662 on this implementation, I get this result:
Differences between the analytic and finite difference derivatives are now on the order of 1e-8, which is much better than before.
@HarrisonKramer I can also add an additional test for the surface normals, which compares the normals with a finite difference approximation. We could also add this test to all Newton-Raphson geometries to prevent this kind of errors in the future. Do you want such tests as part of this PR? And where should they be placed? In
test_geometries.py,test_geometries_extended.py, or somewhere else?Closes #662