fix: Correct transposed axis_correlation_matrix + support fractional axis drops (for AstroImages.jl) - #17
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #17 +/- ##
=======================================
Coverage 86.46% 86.46%
=======================================
Files 18 18
Lines 3376 3376
=======================================
Hits 2919 2919
Misses 457 457 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
icweaver
marked this pull request as ready for review
July 11, 2026 08:30
Member
|
Thanks, looks good, merging now |
icweaver
added a commit
to JuliaAstro/AstroImages.jl
that referenced
this pull request
Jul 13, 2026
Closes the loop on the `slice_wcs` integration discussed in #113 (comment). Depends on JuliaAstro/FITSWCS.jl#17. ## Motivation This PR aims to adopt `FITSWCS.slice_wcs` to simplify the parent-frame bookkeeping here, and also fix several correctness bugs that were unearthed in the process. ## Bugs fixed 1. **Strided slices off by `step-1`**: Legacy mapped slice-local --> parent as `p*step + first - 1`. Correct is `(p-1)*step + first`. The formulas agree only when `step == 1`, so `img[1:2:9, :]` reported world coordinates one parent pixel off. 1. **Frozen refdims off by one pixel**: Dropped axes were fed `dim[1] - 1`. `pixel_to_world(slice; all = true)` reported, e.g., `FREQ = 1.0e9` where the truth for a slice at parent index 2 is `1.001e9`. (Also affected the slice-label values in `implot` titles.) 1. **`parent = true` shifted by one pixel**: `pixel_to_world(img, p; parent = true)` equaled `raw(p .- 1)`, disagreeing with the default path on an unsliced image and shifting WCS-grid extents in the plot recipes by one pixel. 1. **Coupled inverses were garbage**: `world_to_pixel` stuffed frozen *pixel indices* into *world* coordinate slots before inverting. Harmless only when axes are fully separable. Frozen axes now contribute their exact world values (one forward evaluation). 1. **Underdetermined inverses were silent**: Slicing one axis of a celestial lon/lat pair leaves world axes that depend on the dropped pixel axis. `world_to_pixel` now throws a descriptive `ArgumentError` (astropy's `SlicedLowLevelWCS` similarly refuses) instead of returning wrong pixels. 1. **`wcsdims` lost on every slice**: Both `rebuild` methods defaulted to recomputing `wcsdims` from the already-sliced dims (DimensionalData no longer routes slicing through `rebuildsliced`), which broke categorical (Stokes) refdim resolution *and* name-tracked transposition. `rebuild` now preserves the parent `wcsdims`. ## New architecture (`src/wcs.jl`) - `pixel_to_world`: one body for all cases. Expand dims-ordered coords into the full parent frame (kept dims via their lookups, frozen refdims at their parent positions, degenerate header axes at CRPIX), evaluate the full transform, filter to the selected dims (`all = true` returns every axis). - `world_to_pixel` (default): Delegates to a `SlicedWCSTransform` (`FITSWCS.slice_wcs`) describing the current view, guarded by `world_keep == pixel_keep`. - `world_to_pixel(parent = true)`: Unchanged contract for the WCS-grid plot recipes (`naxis` coordinates, WCS axis order), now with exact frozen-axis world values. - Both legacy scatter/gather bodies and the `_world_to_pixel` worker are deleted. Docstrings now document `wcsn` / `all` / `parent` and the 1-based FITS convention. ##⚠️ Breaking / behavior changes - **`world_to_pixel(img, ...)` returns `ndims(img)` slice-local coordinates** in `dims(img)` order (was: `naxis` parent-frame values), making it the actual inverse of `pixel_to_world`. Parent-frame output remains available via `parent = true`. - **`world_to_pixel` throws** on slices that drop a pixel axis the remaining world axes depend on (was: silently wrong values). - **`parent = true` no longer shifted by one pixel**. WCS grid overlays now land at their true positions. - Strided-slice and frozen-axis coordinates corrected per bugs 1–2 above.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #16
1.
axis_correlation_matrixwas transposed 6490546The builder filled the
SMatrixfrom a linearntuplewithi = (k - 1) ÷ N + 1used as the world/row index, butSMatrixfills column-major, so(k - 1) ÷ N + 1is the column. Every entry landed at the transposed position, violating the documented[world, pixel](APE 14) orientation.Why no test caught it: every existing correlation-matrix test uses a symmetric matrix (
[1 1 0; 1 1 0; 0 0 1], celestial pairs, SIP all-true), for which transpose == identity. With asymmetric coupling, e.g.,CD1_3nonzero withoutCD3_1, the matrix was wrong, and since_slice_normalizedderivesworld_keepfrom it,slice_wcskept/dropped the wrong world axes (a shear-coupled cube slice reportedworld_n_dim == 3for a 2-pixel-axis slice instead of 2).Fixed by assigning
w = (k-1) % N + 1(row) /p = (k-1) ÷ N + 1(column), with a comment documenting the column-major fill. Added an asymmetric-CD regression test asserting both the matrix and the resultingpixel_n_dim/world_n_dimafter slicing.2. Fractional (
Real) axis drops (needed by AstroImages.jl)slice_wcspreviously requiredIntegerpositions when dropping an axis. Unlike array indexing, a WCS transform is continuous in pixel space, so freezing an axis at a fractional pixel is well defined, and AstroImages.jl needs it. A droppedDimensionalDatareference dimension can carry a fractional parent-pixel lookup (e.g.,Centered()axes).Widened
_normalize_sliceand_compose_slicesfromIntegertoReal(DropAxisalready storedFloat64), documented in theslice_wcsdocstring, and added tests including composition onto an existing range slice.