Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on January 7
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
There was a problem hiding this comment.
Pull request overview
This PR refactors the PSF (Point Spread Function) calculation methods to align with standard definitions and introduces a new Huygens PSF implementation based on the TOG 2021 paper. The changes separate the previously combined PSF methods into distinct implementations for geometric, exit-pupil propagation, and Huygens approaches.
Key changes:
- Removed the
modeparameter from thepsf()method and created separatepsf_huygens()andpsf_pupil_prop()methods - Implemented a new Huygens-Fresnel integration approach for coherent PSF calculation
- Added
trace2exit_pupil()helper method and improved numerical precision in sensor size calculations
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
deeplens/optics/geometric_surface/plane.py |
Added clarifying parentheses to aperture mask calculation and a comment label |
deeplens/optics/geometric_surface/aperture.py |
Extended init_from_dict to handle additional optional parameters (is_square, pos_xy, vec_local, device) |
deeplens/lens.py |
Removed round() calls from sensor size and pixel size calculations to preserve numerical precision |
deeplens/geolens.py |
Major refactoring: split PSF methods into separate functions, added trace2exit_pupil(), implemented new Huygens PSF calculation, reorganized code and improved documentation |
5_pupil_field.py |
Updated example to demonstrate and compare all three PSF calculation methods with visualization improvements |
Comments suppressed due to low confidence (1)
deeplens/lens.py:122
- Overridden method signature does not match call, where it is passed too many arguments. Overriding method method GeoLens.psf matches the call.
Overridden method signature does not match call, where it is passed an argument named 'spp'. Overriding method method GeoLens.psf matches the call.
Overridden method signature does not match call, where it is passed an argument named 'recenter'. Overriding method method GeoLens.psf matches the call.
Overridden method signature does not match call, where it is passed an argument named 'psf_type'. Overriding method method ParaxialLens.psf matches the call.
Overridden method signature does not match call, where it is passed an argument named 'psf_type'. Overriding method method ParaxialLens.psf matches the call.
Overridden method signature does not match call, where it is passed an argument named 'psf_type'. Overriding method method ParaxialLens.psf matches the call.
Overridden method signature does not match call, where it is passed an argument named 'psf_type'. Overriding method method ParaxialLens.psf matches the call.
Overridden method signature does not match call, where it is passed an argument named 'psf_type'. Overriding method method ParaxialLens.psf matches the call.
Overridden method signature does not match call, where it is passed an argument named 'psf_type'. Overriding method method ParaxialLens.psf matches the call.
Overridden method signature does not match call, where it is passed an argument named 'psf_type'. Overriding method method ParaxialLens.psf matches the call.
Overridden method signature does not match call, where it is passed an argument named 'psf_type'. Overriding method method ParaxialLens.psf matches the call.
def psf(self, points, wvln=0.589, ks=51, **kwargs):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| opl_min = valid_opl.min() | ||
|
|
||
| # Compute distance from each secondary source to each pixel | ||
| batch_size = min(num_valid, 10_000) # Process rays in batches |
There was a problem hiding this comment.
The batch processing in the Huygens integration could be inefficient. The batch_size is hardcoded to 10,000, which may be too small for modern GPUs with large memory. Consider making this configurable or dynamically computing it based on available memory and ks size to maximize GPU utilization.
| self.real_rfov = self.rfov | ||
| self.real_dfov = self.dfov | ||
| print(f"Ray tracing to compute fov failed, set real_rfov to {self.rfov}.") | ||
| print(f"Failed to calculate distored FoV by ray tracing, use effective FoV {self.rfov} rad.") |
There was a problem hiding this comment.
The error message has been improved to clarify that it's the "distorted FoV" calculation that failed, but the message uses "distored" which appears to be a typo. The correct spelling should be "distorted".
| print(f"Failed to calculate distored FoV by ray tracing, use effective FoV {self.rfov} rad.") | |
| print(f"Failed to calculate distorted FoV by ray tracing, use effective FoV {self.rfov} rad.") |
| ray (Ray object): Ray object. | ||
| """ | ||
| ray = self.trace2sensor(ray) | ||
| pupil_z, _ = self.get_exit_pupil() |
There was a problem hiding this comment.
The trace2exit_pupil method propagates rays to the exit pupil z-position using ray.prop_to(pupil_z), but it doesn't validate that pupil_z is valid or that the propagation succeeds. Consider adding validation or documentation about what happens when the exit pupil calculation fails or returns an unexpected value.
| pupil_z, _ = self.get_exit_pupil() | |
| pupil_z, _ = self.get_exit_pupil() | |
| # Validate exit pupil position before propagation to avoid using | |
| # an invalid or non-finite z value, which could cause numerical | |
| # issues or unexpected behavior in ray propagation. | |
| if pupil_z is None: | |
| raise ValueError( | |
| "Exit pupil z-position is None. Exit pupil calculation failed." | |
| ) | |
| # Handle scalar and array-like values (e.g., numpy arrays) | |
| try: | |
| is_finite = bool(np.all(np.isfinite(pupil_z))) | |
| except TypeError: | |
| # Fallback for non-numpy types; for simple scalars math.isfinite works. | |
| is_finite = isinstance(pupil_z, (int, float)) and math.isfinite(pupil_z) | |
| if not is_finite: | |
| raise ValueError( | |
| f"Exit pupil z-position is not finite: {pupil_z!r}. " | |
| "Exit pupil calculation returned an unexpected value." | |
| ) |
| (torch.abs(new_o[..., 0]) < (self.w / 2)) | ||
| & (torch.abs(new_o[..., 1]) < (self.h / 2)) |
There was a problem hiding this comment.
The added parentheses around self.w / 2 and self.h / 2 are redundant and don't change the behavior since division already has higher precedence than comparison operators. While this doesn't introduce a bug, it's unnecessary and could be misleading.
| (torch.abs(new_o[..., 0]) < (self.w / 2)) | |
| & (torch.abs(new_o[..., 1]) < (self.h / 2)) | |
| (torch.abs(new_o[..., 0]) < self.w / 2) | |
| & (torch.abs(new_o[..., 1]) < self.h / 2) |
| 2 * self.r_sensor * self.sensor_res[0] / diam_res, | ||
| 2 * self.r_sensor * self.sensor_res[1] / diam_res, | ||
| ) | ||
| self.pixel_size = round(self.sensor_size[0] / self.sensor_res[0], 4) | ||
| self.pixel_size = self.sensor_size[0] / self.sensor_res[0] |
There was a problem hiding this comment.
Removing round() from sensor size and pixel size calculations increases numerical precision, which is beneficial. However, this could introduce minor backward compatibility issues if downstream code or tests depend on the exact rounded values. Ensure that this change doesn't break any existing functionality that may rely on these specific values.
| pointc[0, 0] + x_coords, | ||
| pointc[0, 1] + y_coords, | ||
| indexing="xy" | ||
| ) # [ks, ks] each |
There was a problem hiding this comment.
The psf_huygens function only supports a single point at a time (indexing pointc[0, 0] and pointc[0, 1]), but the function signature and documentation suggest it should support batches of points. Either add batch support by iterating over points, or document that only single points are supported and add validation to reject batched inputs.
| # 1. Geometric PSF: incoherent intensity ray tracing | ||
| # 2. Exit-pupil PSF: coherent ray tracing to exit pupil, then free-space propagation with ASM | ||
| # 3. Huygens PSF: coherent ray tracing to exit pupil, then Huygens-Fresnel integration |
There was a problem hiding this comment.
The documentation describes three PSF methods but only provides accurate information for the third one (exit-pupil PSF). The comment should clarify that "Exit-pupil PSF" refers to psf_pupil_prop or psf_coherent, and "Huygens PSF" refers to psf_huygens. Consider adding cross-references to the actual method names.
| # 1. Geometric PSF: incoherent intensity ray tracing | |
| # 2. Exit-pupil PSF: coherent ray tracing to exit pupil, then free-space propagation with ASM | |
| # 3. Huygens PSF: coherent ray tracing to exit pupil, then Huygens-Fresnel integration | |
| # 1. Geometric PSF (`psf`): incoherent intensity ray tracing | |
| # 2. Exit-pupil PSF (`psf_pupil_prop` / `psf_coherent`): coherent ray tracing to exit pupil, then free-space propagation with ASM | |
| # 3. Huygens PSF (`psf_huygens`): coherent ray tracing to exit pupil, then Huygens-Fresnel integration |
| psf_coherent = lens.psf_coherent(torch.tensor([0.0, 0.4, -10000.0]), ks=64) | ||
| save_image(psf_coherent, "./psf_coherent.png", normalize=True) | ||
| psf_incoherent = lens.psf(torch.tensor([0.0, 0.0, -10000.0]), ks=64) | ||
|
|
||
| psf_incoherent = lens.psf(torch.tensor([0.0, 0.4, -10000.0]), ks=64) | ||
| save_image(psf_incoherent, "./psf_incoherent.png", normalize=True) | ||
|
|
||
| psf_huygens = lens.psf_huygens(torch.tensor([0.0, 0.4, -10000.0]), ks=64) | ||
| save_image(psf_huygens, "./psf_huygens.png", normalize=True) |
There was a problem hiding this comment.
The example now uses different point sources for geometric, Huygens, and coherent PSFs (changed from [0.0, 0.0, -10000.0] to [0.0, 0.4, -10000.0]). While this makes the comparison more interesting for off-axis points, it would be clearer to compare all three methods using the same point source first, then optionally show off-axis behavior separately.
| assert spp >= 1_000_000, ( | ||
| f"Ray sampling {spp} is too small for coherent ray tracing, which may lead to inaccurate simulation." |
There was a problem hiding this comment.
The assertion message uses an f-string to include the actual spp value, which is helpful. However, the minimum required spp is hardcoded as 1,000,000 in the assertion. This should match the constant SPP_COHERENT (16,777,216) or use a named constant for clarity and consistency.
| assert spp >= 1_000_000, ( | |
| f"Ray sampling {spp} is too small for coherent ray tracing, which may lead to inaccurate simulation." | |
| assert spp >= SPP_COHERENT, ( | |
| f"Ray sampling {spp} is too small for coherent ray tracing (minimum {SPP_COHERENT}), which may lead to inaccurate simulation." |
| [1] "End-to-End Hybrid Refractive-Diffractive Lens Design with Differentiable Ray-Wave Model", SIGGRAPH Asia 2024. | ||
|
|
||
| Note: | ||
| [1] This function is similar to ZEMAX FFT_PSF but implement free-space propagation with Angular Spectrum Method (ASM) rathar than FFT transform. Free-space propagation using ASM is more accurate than doing FFT, because FFT (as used in ZEMAX) assumes far-field condition (e.g., chief ray perpendicular to image plane). |
There was a problem hiding this comment.
The Note section states "This function is similar to ZEMAX FFT_PSF but implement free-space propagation with Angular Spectrum Method (ASM) rathar than FFT transform." The word "rathar" should be "rather".
| [1] This function is similar to ZEMAX FFT_PSF but implement free-space propagation with Angular Spectrum Method (ASM) rathar than FFT transform. Free-space propagation using ASM is more accurate than doing FFT, because FFT (as used in ZEMAX) assumes far-field condition (e.g., chief ray perpendicular to image plane). | |
| [1] This function is similar to ZEMAX FFT_PSF but implement free-space propagation with Angular Spectrum Method (ASM) rather than FFT transform. Free-space propagation using ASM is more accurate than doing FFT, because FFT (as used in ZEMAX) assumes far-field condition (e.g., chief ray perpendicular to image plane). |
| # Shape of [N, 2], un-normalized physical coordinates | ||
| pointc_chief_ray = self.psf_center(point_obj, method="chief_ray") | ||
| raise ValueError("Points must be of shape [1, 3].") | ||
| single_point = False |
There was a problem hiding this comment.
Incorrect shape validation with unreachable dead code
The shape validation in psf_huygens is contradictory and contains unreachable code. The error message states "Points must be of shape [1, 3]" but the code actually rejects tensors of shape [1, 3] since len(points.shape) == 2 routes to the else branch which raises ValueError. Only 1D tensors of shape [3] are accepted. Additionally, the line single_point = False after the raise statement is unreachable dead code. This prevents users from passing valid 2D single-point tensors like [[0.0, 0.0, -10000.0]].
|
|
||
| # Compute distance from each secondary source to each pixel | ||
| batch_size = min(num_valid, 10_000) # Process rays in batches | ||
| for batch_start in range(0, num_valid, batch_size): |
There was a problem hiding this comment.
Crash on empty valid rays in Huygens PSF
In psf_huygens, when all rays are blocked/vignetted during tracing (resulting in zero valid rays), the function crashes with unhelpful errors. Specifically, valid_opl.min() on an empty tensor raises RuntimeError, and if that were bypassed, range(0, num_valid, batch_size) where batch_size=0 raises ValueError: range() arg 3 must not be zero. This edge case can occur with extreme field positions or unusual lens configurations. When recenter=True, the error is caught earlier in psf_center, but when recenter=False the crash occurs here without a clear error message.
Note
Implements coherent Huygens PSF and streamlines PSF APIs while improving pupil and aperture handling.
GeoLens.psf_huygens(coherent exit‑pupil tracing + Huygens–Fresnel integration), normalizes/flip outputs;psf_coherentnow delegates topsf_pupil_prop;psfis geometric-only withrecentersupport; introducestrace2exit_pupilpupil_fieldto usetrace2exit_pupil, supportsrecenter, and returnspsf_center; standardizes intensity normalization across PSF pathsAperture.init_from_dictacceptsis_square/pos_xy/vec_local/device;Plane.intersectenforces square/round aperture masks; entrance‑pupil radius accounts for square apertures;Lens.set_sensor_resavoids rounding for precisesensor_size/pixel_size5_pupil_field.pynow computes and comparespsf(geometric),psf_huygens, andpsf_coherent/psf_pupil_prop, and plots center‑line profilesWritten by Cursor Bugbot for commit 6ac0358. This will update automatically on new commits. Configure here.