Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors autoarray border relocation to use a PCA-derived ellipse model for relocating grid points outside a mask border, replacing the prior nearest-border-pixel approach and removing the use_sparse_operator / Numba-JIT relocation pathway.
Changes:
- Add PCA ellipse fitting + ellipse-boundary relocation utilities and update
BorderRelocatorto use them. - Remove
use_sparse_operatorplumbing and delete the Numbarelocated_grid_via_jit_fromimplementation. - Update Delaunay mesh wiring and unit tests to match the new relocation outputs / types.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
autoarray/inversion/pixelization/border_relocator.py |
Introduces PCA/ellipse relocation functions and switches BorderRelocator to the new method. |
autoarray/inversion/pixelization/mesh/delaunay.py |
Adjusts Delaunay mapper grid plumbing around relocation inputs. |
autoarray/inversion/inversion/imaging_numba/inversion_imaging_numba_util.py |
Removes the old JIT relocation function. |
autoarray/dataset/imaging/dataset.py |
Removes passing use_sparse_operator into GridsDataset. |
autoarray/dataset/interferometer/dataset.py |
Removes passing use_sparse_operator into GridsDataset. |
autoarray/dataset/grids.py |
Removes use_sparse_operator storage and forwarding into BorderRelocator. |
test_autoarray/inversion/pixelization/test_border_relocator.py |
Updates expected relocated values for the new ellipse method. |
test_autoarray/inversion/pixelization/mesh/test_abstract.py |
Updates mesh-grid test setup to align with updated types. |
autoarray/__init__.py |
Minor whitespace cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| relocated_mesh_grid = self.relocated_mesh_grid_from( | ||
| border_relocator=border_relocator, | ||
| source_plane_data_grid=relocated_grid.over_sampled, | ||
| source_plane_data_grid=Grid2DIrregular(relocated_grid.over_sampled), |
There was a problem hiding this comment.
mapper_grids_from() passes Grid2DIrregular(relocated_grid.over_sampled) into relocated_mesh_grid_from(), but AbstractMesh.relocated_mesh_grid_from()/BorderRelocator.relocated_mesh_grid_from() expect the pixel-centre Grid2D so they can index border points via border_slim. Using the over-sampled irregular grid changes the indexing basis and will fit the ellipse to the wrong points (and may relocate the mesh incorrectly). Pass relocated_grid (or the original source_plane_data_grid) instead, or update BorderRelocator.relocated_mesh_grid_from() to explicitly use sub_border_slim when working with over-sampled grids.
| source_plane_data_grid=Grid2DIrregular(relocated_grid.over_sampled), | |
| source_plane_data_grid=relocated_grid, |
| self, | ||
| mask: Mask2D, | ||
| sub_size: Union[int, Array2D], | ||
| use_sparse_operator: bool = False, | ||
| ): | ||
| """ | ||
| Relocates source plane coordinates that trace outside the mask’s border in the source-plane back onto the |
There was a problem hiding this comment.
The BorderRelocator docstring (below this signature) still describes the old nearest-border-pixel / radial-distance relocation behavior, but the implementation now uses PCA-derived ellipse parameters (ellipse_params_via_border_pca_from + relocated_grid_via_ellipse_border_from). Please update the docstring so it matches the new ellipse-based relocation logic and doesn’t mislead users.
| def relocated_grid_from(self, grid: Grid2D, xp=np) -> Grid2D: | ||
| """ | ||
| Relocate the coordinates of a grid to the border of this grid if they are outside the border, where the |
There was a problem hiding this comment.
relocated_grid_from()’s docstring describes the previous nearest-border-pixel relocation algorithm, but the function now fits an ellipse via PCA and scales outside points to the ellipse boundary. Please update the method documentation to reflect the new behavior (including how the border is modeled and what guarantees it provides).
| class EllipseParams: | ||
|
|
||
| return relocated_grid | ||
| def __init__(self, origin, a, b, phi): | ||
|
|
||
| self.origin = origin | ||
| self.a = a | ||
| self.b = b | ||
| self.phi = phi |
There was a problem hiding this comment.
EllipseParams is introduced but not used anywhere in the module or codebase. This adds dead code/maintenance surface; either use it as the return type of ellipse_params_via_border_pca_from() / input to the relocation function, or remove it until it’s needed.
1 participant
This pull request refactors the border relocation logic in the
autoarraypackage to use an ellipse-based PCA approach for relocating grid points outside the mask border, replacing the previous method that relied on finding the nearest border pixel.This uses approximately 3 times or more less VRAM than the previous implementation on GPU, and runs faster.
The changes remove the
use_sparse_operatoroption and associated JIT/Numba code, streamline theBorderRelocator, and update related tests and usages.Border relocation algorithm update:
ellipse_params_via_border_pca_fromandrelocated_grid_via_ellipse_border_from) for relocating grid points outside the border, providing a more robust and vectorized solution. [1] [2] [3]relocated_grid_via_jit_from) and all conditional logic foruse_sparse_operator, simplifying the codebase and ensuring a single, consistent relocation method. [1] [2] [3] [4] [5] [6] [7]Class and interface adjustments:
BorderRelocatorand its interface by removing theuse_sparse_operatorparameter and related state.EllipseParamsclass for storing ellipse parameters, improving code clarity.Test and usage updates:
Grid2DIrregularobjects where needed. [1] [2] [3] [4]