Skip to content

BUG: ULTRA l2 downsample counts#2939

Merged
lacoak21 merged 2 commits intoIMAP-Science-Operations-Center:devfrom
lacoak21:ultra_l2_counts_downsample_bufix
Apr 9, 2026
Merged

BUG: ULTRA l2 downsample counts#2939
lacoak21 merged 2 commits intoIMAP-Science-Operations-Center:devfrom
lacoak21:ultra_l2_counts_downsample_bufix

Conversation

@lacoak21
Copy link
Copy Markdown
Contributor

@lacoak21 lacoak21 commented Apr 8, 2026

Change Summary

closes #2938

Overview

Bug fix.

To convert the counts array from ring to nested, I should use nest2ring because this returns what each ring pixels is at each nested index. Using the reversal was scrambling counts.

ring2nest gives, for each ring pixel, what is the nested pixel ind. But we want for each nested pixel what is the ring pixel ind.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a ULTRA L2 counts downsampling bug caused by using the wrong HEALPix ring↔nested index mapping, which could scramble per-pixel counts during reordering.

Changes:

  • Corrects ring→nested reindexing by using hp.nest2ring(...) when the dataset is in ring ordering.
  • Corrects nested→ring reindexing by using hp.ring2nest(...) when converting the binned result back.
  • Updates inline comments to document the intended conversion behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 655 to 663
# Get counts in nested ordering. In nested ordering, the
# pixels that need to be binned together to go from the counts nside to
# the pset nside are contiguous in the array.
# Use nest2ring to get the indices to convert from ring to nest ordering if
# necessary.
if not self.nested:
counts_n = counts[
:, hp.ring2nest(counts_nside, np.arange(counts_n_pix))
:, hp.nest2ring(counts_nside, np.arange(counts_n_pix))
]
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

The existing test_downsample_counts assertion only checks that the total counts sum is preserved after downsampling, but this bug fix specifically corrects pixel ordering (ring↔nested index mapping). A scrambling regression could still pass the current test; please add a regression assertion that validates per-pixel placement (e.g., construct a synthetic counts array with a unique per-pixel pattern, run downsample_counts, and verify the expected pixel-to-pixel mapping for both ring and nested cases).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@subagonsouth subagonsouth left a comment

Choose a reason for hiding this comment

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

👍

@lacoak21 lacoak21 merged commit 8595fda into IMAP-Science-Operations-Center:dev Apr 9, 2026
18 checks passed
@lacoak21 lacoak21 deleted the ultra_l2_counts_downsample_bufix branch April 9, 2026 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ULTRA L2 downsample counts BUG

3 participants