ULTRA l1c counts nside update#2857
ULTRA l1c counts nside update#2857lacoak21 wants to merge 7 commits intoIMAP-Science-Operations-Center:devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates ULTRA L1C count-map handling to mitigate aliasing by writing counts on a finer HEALPix grid (higher nside) and then downsampling to the PSET grid during ENA map processing (L2-facing UltraPointingSet).
Changes:
- Introduces a separate
counts_pixel_indexcoordinate/dimension for L1Ccountsand sets a fixed higher-resolutionL1C_COUNTS_NSIDE. - Updates L1C PSET generation to bin
countsat the highernsidewhile keeping other variables on the nominal PSET grid. - Adds
UltraPointingSet.downsample_counts()and corresponding test updates/mocks.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| imap_processing/ultra/utils/ultra_l1_utils.py | Adds counts_pixel_index coord and changes counts dims to use it. |
| imap_processing/ultra/l1c/spacecraft_pset.py | Bins counts at L1C_COUNTS_NSIDE and adds counts_pixel_index. |
| imap_processing/ultra/l1c/helio_pset.py | Same as spacecraft PSET, for helio PSET generation. |
| imap_processing/ultra/constants.py | Adds UltraConstants.L1C_COUNTS_NSIDE = 128. |
| imap_processing/tests/ultra/mock_data.py | Updates mock L1C PSET generator to produce higher-res counts than other variables. |
| imap_processing/tests/ena_maps/test_ena_maps.py | Adds downsampling test and patches init test to bypass downsampling. |
| imap_processing/ena_maps/utils/coordinates.py | Introduces CoordNames.COUNTS_HEALPIX_INDEX. |
| imap_processing/ena_maps/ena_maps.py | Adds UltraPointingSet.downsample_counts() and calls it in __init__. |
| imap_processing/cdf/config/imap_ultra_l1c_variable_attrs.yaml | Updates counts.DEPEND_2 to counts_pixel_index. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Mock the downsample_counts method to avoid dimension bugs | ||
| # since this is a dummy cdf, the dimensions are not present. | ||
| with patch.object( | ||
| ena_maps.UltraPointingSet, "downsample_counts", lambda self: None | ||
| ): | ||
| ultra_pset_from_str = ena_maps.UltraPointingSet(cdf_filepath) | ||
| ultra_pset_from_path = ena_maps.UltraPointingSet(Path(cdf_filepath)) | ||
| ultra_pset_from_dataset = ena_maps.UltraPointingSet(ultra_pset) |
There was a problem hiding this comment.
This test now patches out UltraPointingSet.downsample_counts() to avoid dimension errors when reading the dummy CDF. That means test_init_cdf no longer exercises the real initialization path (including the new downsampling/renaming behavior) for either in-memory datasets or CDF-loaded datasets. It would be better to construct the dummy dataset/CDF with the expected counts_pixel_index coordinate (or make downsample_counts robust to missing dims) so the test validates the actual behavior instead of bypassing it.
subagonsouth
left a comment
subagonsouth
left a comment
There was a problem hiding this comment.
Wow, I am surprised that Ultra is changing something this fundamental this late in the game! Overall, this looks good. I think that Copilot has some valuable feedback in places. I just have a couple of suggestions.
Agreed. I met with Tenzin this morning to chat about all the last minute requests coming in 😨. Oops also I realized I had stashed code - sorry for some unfinished stuff |
subagonsouth
left a comment
subagonsouth
left a comment
There was a problem hiding this comment.
This looks good to me.
Change Summary
closes #2696
Overview
Counts at l1c need to be binned at a higher resolution and then downsampled at l2.
File changes
Testing
Add tests to check downsampling.