Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates a shared geometry helper used to convert radial coordinates back into Cartesian coordinates, aiming to simplify angle handling by using direct trigonometric calls, plus a small call-site cleanup for readability.
Changes:
- Updated
SphProfile._cartesian_grid_via_radial_fromto computecos_theta/sin_thetadirectly viaxp.cos/xp.sin. - Reordered keyword arguments in an
IsothermalSph.deflections_yx_2d_fromcall for clarity (no functional change).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
autogalaxy/profiles/geometry_profiles.py |
Simplifies angle-to-(sin,cos) computation inside _cartesian_grid_via_radial_from. |
autogalaxy/profiles/mass/total/isothermal.py |
Reorders keyword arguments in a helper call for readability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cos_theta = xp.cos(grid_angles) | ||
| sin_theta = xp.sin(grid_angles) |
There was a problem hiding this comment.
_cartesian_grid_via_radial_from previously delegated to self.angle_to_profile_grid_from(...), which is overridden by EllProfile to account for the profile’s position angle. Replacing it with xp.cos/sin(grid_angles) bypasses that override and changes behavior for any subclass with a non-zero angle that calls this helper. Consider restoring the call to self.angle_to_profile_grid_from(...), or (if the intent is to always ignore the profile angle because the grid is already in the profile reference frame) add an explicit comment and/or use a clearly-named helper to avoid silently breaking polymorphism.
| cos_theta = xp.cos(grid_angles) | |
| sin_theta = xp.sin(grid_angles) | |
| # Use angle_to_profile_grid_from so subclasses (e.g. with non-zero position angle) | |
| # can override how angles are converted to Cartesian directions. | |
| cos_theta, sin_theta = self.angle_to_profile_grid_from( | |
| grid_angles=grid_angles, xp=xp, **kwargs | |
| ) |
| grid_angles = xp.arctan2(grid.array[:, 0], grid.array[:, 1]) | ||
| cos_theta, sin_theta = self.angle_to_profile_grid_from( | ||
| grid_angles=grid_angles, xp=xp | ||
| ) | ||
|
|
||
| cos_theta = xp.cos(grid_angles) | ||
| sin_theta = xp.sin(grid_angles) | ||
|
|
||
| return xp.multiply(radius[:, None], xp.vstack((sin_theta, cos_theta)).T) |
There was a problem hiding this comment.
This change alters the angle-to-(sin,cos) conversion path used by _cartesian_grid_via_radial_from, but there’s no unit test covering this helper (e.g. verifying the returned Cartesian vectors for known input grids/radii, including a rotated EllProfile case if overrides are expected to apply). Adding a focused test would help prevent regressions in coordinate conversion.
This pull request makes a minor change to the calculation of Cartesian grid coordinates from radial coordinates in
geometry_profiles.py.This is motivated by a JAX tracing issue which leads to NaN in the defleciton angles of certain mass profile like the
IsothermalSph. This occurs only on GPU and when tracing via a full tracer. This PR does not fix the underlying issue, which is likely a JAX tracing bug, but is a hot fix which means lens modeling runs OK for now.The main update is to use direct trigonometric functions for angle conversion, simplifying the code and potentially improving performance.
Improvements to coordinate calculation:
autogalaxy/profiles/geometry_profiles.py: Replaced the call toangle_to_profile_grid_fromwith direct calculation ofcos_thetaandsin_thetausingxp.cosandxp.sin, removing unnecessary indirection.Minor code cleanup:
autogalaxy/profiles/mass/total/isothermal.py: Adjusted the order of arguments in the call to_cartesian_grid_via_radial_fromfor clarity, but no functional change.