Skip to content

Extract independent cleanup fixes#151

Merged
nabobalis merged 1 commit into
mainfrom
independent-cleanups-from-main
Jun 12, 2026
Merged

Extract independent cleanup fixes#151
nabobalis merged 1 commit into
mainfrom
independent-cleanups-from-main

Conversation

@nabobalis

@nabobalis nabobalis commented Jun 12, 2026

Copy link
Copy Markdown
Member

Summary by Sourcery

Improve cosmic ray removal, WCS generation, and visualization slider labeling while adding dask support and updating dependencies.

New Features:

  • Support Dask-backed data and masks in astroscrappy-based cosmic ray removal.
  • Automatically shorten WCS-derived slider labels for IRIS visualizations based on axis semantics.

Bug Fixes:

  • Handle NaN values and in-mask combination correctly per-frame in astroscrappy cosmic ray detection.
  • Ensure velocity-space WCS construction is robust by building it directly from array shape and velocity grid instead of modifying an existing WCS header.
  • Reject use of the rsliding cosmic-ray removal method on Dask-backed cubes that cannot be fully loaded into memory.

Enhancements:

  • Relax type annotations in rsliding cosmic ray removal helpers to work with non-NumPy array backends.
  • Standardize default raster sequence slider labels to sentence case for consistency.

Build:

  • Add runtime dependencies on dask[array] and gwcs and remove the docs-only dependency on dask[distributed].

Tests:

  • Add tests covering Dask-backed astroscrappy cosmic ray removal and validation of rsliding behavior with Dask cubes.
  • Add visualization tests to assert shortened default slider labels and respect for custom slider labels.

@sourcery-ai

sourcery-ai Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

Refactors cosmic ray removal to be robust for NaNs and dask-backed cubes, simplifies velocity WCS construction for red/blue profile cubes, standardizes/shortens IRIS visualization slider labels, and adjusts runtime dependencies and tests accordingly.

File-Level Changes

Change Details Files
Make astroscrappy-based cosmic ray removal robust to NaNs and support dask-backed cubes while rejecting incompatible rsliding usage.
  • Generalize _remove_cosmic_rays_rsliding input types to accept non-NumPy array data.
  • Rewrite _remove_cosmic_rays_astroscrappy to iterate over leading dimensions, build per-frame masks including NaNs and optional inmask, and zero out masked pixels before calling detect_cosmics.
  • Detect dask-backed cubes in remove_cosmic_rays, forbid rsliding for such cubes with a clear error, and avoid global mask materialization by passing through the cube mask directly for astroscrappy.
  • Adjust working mask construction to be skipped for dask-backed data and retain NaN-handling only in the in-memory path.
irispy/utils/cosmic_rays.py
Extend tests to cover dask-backed cosmic ray removal behavior and validation of unsupported methods.
  • Introduce a simple FakeCube helper used to wrap data/mask for tests and to_nddata conversion.
  • Add tests that validate astroscrappy cosmic ray removal on dask-backed cubes, including NaN handling and inmask propagation.
  • Add a test ensuring rsliding raises a ValueError when used with dask-backed cubes.
  • Keep existing tests for missing optional dependencies intact while integrating with new helper.
irispy/utils/tests/test_cosmic_rays.py
Construct a self-contained velocity WCS for red/blue profile cubes without relying on the base WCS.
  • Replace FITS-header-based _make_velocity_wcs implementation with one that creates a new astropy.wcs.WCS with specified naxis and velocity axis properties only.
  • Compute the velocity axis index in WCS order, set VELO CTYPE, km/s units, and basic linear WCS parameters from the velocity grid, and attach array_shape.
  • Update _make_profile_cube to call the new _make_velocity_wcs signature that no longer takes the base cube WCS.
irispy/utils/red_blue.py
Standardize and shorten IRIS visualization slider labels based on WCS coordinate types, and tweak default sequence labels.
  • Extend LAT_LABELS and LON_LABELS lists to include lowercase helioprojective labels and introduce TIME_LABELS for time axes.
  • Add _shorten_slider_label helper that collapses wavelength/time/scan-related composite labels to simple canonical forms.
  • Override IRISArrayAnimatorWCS._compute_slider_labels_from_wcs to post-process parent labels through _shorten_slider_label.
  • Adjust IRISSequenceAnimator defaults to use sentence case slider labels ("Raster step", "Scan number").
irispy/visualization.py
Add visualization tests to ensure slider label shortening and user overrides behave as expected.
  • Add an SJI plotting test asserting the default slider labels collapse to ["Time"] for the test cube.
  • Add an SJI plotting test asserting custom slider_labels passed into plot are respected unchanged and still close figures to avoid leakage.
irispy/tests/test_sji.py
Update project dependencies to include required runtime packages for dask-backed operations and gwcs, and simplify docs extras.
  • Declare dask[array]>=2024.6.0 and gwcs>=1.0.0 as core runtime dependencies.
  • Remove dask[distributed] from the docs extras group, keeping other documentation dependencies unchanged.
pyproject.toml

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@nabobalis nabobalis force-pushed the independent-cleanups-from-main branch from 8354bdf to 9e13c50 Compare June 12, 2026 23:39
@nabobalis nabobalis marked this pull request as ready for review June 12, 2026 23:40
@nabobalis nabobalis added the No Changelog Entry Needed Skip all changelog checks. label Jun 12, 2026

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • The astroscrappy code path eagerly converts each dask chunk to NumPy and stores results in preallocated NumPy arrays, so the output cube is no longer dask-backed; if preserving laziness is desired, consider returning a dask array (e.g. via map_blocks) or at least documenting that this operation materializes the full cube in memory.
  • In _shorten_slider_label, set(LON_LABELS + LAT_LABELS) is recomputed on every call; consider creating a module-level SCAN_LABELS set instead to avoid repeated allocation on frequently updated sliders.
  • The new _make_velocity_wcs creates a fresh WCS with only the velocity axis configured, dropping any contextual WCS information from the original cube; if some non-velocity axes metadata should be preserved (e.g. observer or coordinate frames), consider explicitly copying over the relevant parts from the input cube WCS.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The astroscrappy code path eagerly converts each dask chunk to NumPy and stores results in preallocated NumPy arrays, so the output cube is no longer dask-backed; if preserving laziness is desired, consider returning a dask array (e.g. via map_blocks) or at least documenting that this operation materializes the full cube in memory.
- In `_shorten_slider_label`, `set(LON_LABELS + LAT_LABELS)` is recomputed on every call; consider creating a module-level `SCAN_LABELS` set instead to avoid repeated allocation on frequently updated sliders.
- The new `_make_velocity_wcs` creates a fresh WCS with only the velocity axis configured, dropping any contextual WCS information from the original cube; if some non-velocity axes metadata should be preserved (e.g. observer or coordinate frames), consider explicitly copying over the relevant parts from the input cube WCS.

## Individual Comments

### Comment 1
<location path="irispy/utils/cosmic_rays.py" line_range="65-74" />
<code_context>
+        frame_mask = np.zeros(frame.shape, dtype=bool) if mask is None else np.asarray(mask[index], dtype=bool)
+        if np.issubdtype(frame.dtype, np.floating):
+            frame_mask = frame_mask | np.isnan(frame)
+        if inmask is not None:
+            frame_mask = frame_mask | np.asarray(inmask[index], dtype=bool)
+        frame[frame_mask] = 0.0
+        detected_mask, cleaned_frame = astroscrappy.detect_cosmics(frame, inmask=frame_mask, **method_kwargs)
</code_context>
<issue_to_address>
**issue (bug_risk):** Indexing `inmask` per frame changes semantics for 2D masks on >2D data and may break previous broadcasting behavior.

Previously, a 2D `inmask` was combined with `mask` without indexing, effectively applying the same mask to every frame in a higher‑dimensional cube. By indexing `inmask[index]`, a 2D `inmask` for an n>2D cube will now either fail or (worse) mask a different set of pixels than before. If you want to preserve the old behavior, consider checking `inmask.ndim`/shape and only index when it matches `data`’s leading dimensions, otherwise broadcast it explicitly across frames.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread irispy/utils/cosmic_rays.py
@nabobalis nabobalis force-pushed the independent-cleanups-from-main branch from 9e13c50 to d058228 Compare June 12, 2026 23:47
@nabobalis nabobalis force-pushed the independent-cleanups-from-main branch from d058228 to 8eec915 Compare June 12, 2026 23:52
@nabobalis nabobalis merged commit 525f070 into main Jun 12, 2026
19 of 21 checks passed
@nabobalis nabobalis deleted the independent-cleanups-from-main branch June 12, 2026 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

No Changelog Entry Needed Skip all changelog checks.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant