Skip to content

3309 hi l2 add l2 map variables - #3365

Open
tmplummer wants to merge 4 commits into
IMAP-Science-Operations-Center:devfrom
tmplummer:3309-hi-l2---add-l2-map-variables
Open

3309 hi l2 add l2 map variables#3365
tmplummer wants to merge 4 commits into
IMAP-Science-Operations-Center:devfrom
tmplummer:3309-hi-l2---add-l2-map-variables

Conversation

@tmplummer

Copy link
Copy Markdown
Contributor

Adds several new variables to IMAP-Hi L2 map products that were previously computed internally but dropped (or, in one case, silently lost) before final CDF output:

  • ena_count — renamed from counts at the L1C→L2 rename step, fixing a bug where it had no CDF attribute definition and was being silently dropped from the final map.
  • bg_rate / bg_rate_sys_err — no longer discarded as "intermediate"; now combined across calibration_prod (inverse-variance weighted) and kept in the final product.
  • ena_intensity_background_systematic_err and ena_intensity_calibration_systematic_err — split out from what was previously an internal, discarded computation. ena_intensity_sys_err (unchanged, still present) is now their quadrature combination.
  • The background systematic term is rescaled by the same flux-correction ratio applied to ena_intensity, keeping it consistent with the corrected intensity (previously it wasn't corrected at all).
  • combine_maps() (ram/anti heliocentric-frame combination) extended to properly combine all the new/renamed variables instead of silently dropping the anti-map's contribution.
  • New CDF attribute entries added to the shared imap_enamaps_l2-common/imap_enamaps_l2-rectangular YAML configs for the two new systematic-error variables.
  • Test coverage updated/added in test_hi_l2.py and test_ena_maps.py accordingly.

Closes: #3309

…na_intensity_background_systematic_err, ena_intensity_calibration_systematic_err, bg_rate, bg_rate_sys_err

Copilot AI 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.

Pull request overview

This PR updates IMAP-Hi L2 ENA map production to retain additional diagnostic and uncertainty variables through to final CDF output, including a rename of the counts variable and new split systematic-error components, and extends map-combination logic to include these variables.

Changes:

  • Rename map-level event counts from counts to ena_count and propagate the rename through Hi processing and ENA map utilities/tests.
  • Preserve and propagate bg_rate / bg_rate_sys_err, and add ena_intensity_background_systematic_err and ena_intensity_calibration_systematic_err (with ena_intensity_sys_err as their quadrature sum).
  • Add/update CDF attribute config entries and expand tests for the new/renamed variables.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
imap_processing/hi/hi_l2.py Renames countsena_count, adds split systematic errors, combines new vars across calibration products, and extends ram/anti combine logic.
imap_processing/ena_maps/ena_maps.py Maps PSET counts to ena_count in HiPointingSet renames.
imap_processing/ena_maps/utils/naming.py Adds CATDESC mappings for the new systematic-error variables.
imap_processing/cdf/config/imap_enamaps_l2-common_variable_attrs.yaml Adds common CDF attribute entries for the new systematic-error variables.
imap_processing/cdf/config/imap_enamaps_l2-rectangular_variable_attrs.yaml Adds rectangular-map dependency pointers for the new systematic-error variables.
imap_processing/tests/hi/test_hi_l2.py Updates/extends Hi L2 tests for renamed counts and new systematic-error variables; adds coverage for flux-ratio scaling.
imap_processing/tests/ena_maps/test_ena_maps.py Updates ENA maps tests for the ena_count rename and expected variables.
Suppressed comments (1)

imap_processing/tests/hi/test_hi_l2.py:1484

  • combine_maps() now combines bg_rate, bg_rate_sys_err, and the two split systematic-error components, but this test only asserts ena_count/exposure_factor. Add explicit assertions for the exposure-weighted results of the newly combined variables (with differing ram/anti values from the fixture) to prevent regressions.
    expected_counts = 100 + 120  # 100 + (100 + 20)
    np.testing.assert_array_almost_equal(
        result.data_1d["ena_count"].values,
        np.ones_like(result.data_1d["ena_count"].values) * expected_counts,
    )

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

Comment thread imap_processing/hi/hi_l2.py
Comment thread imap_processing/hi/hi_l2.py
Comment thread imap_processing/hi/hi_l2.py
Comment thread imap_processing/tests/hi/test_hi_l2.py
tmplummer and others added 2 commits August 4, 2026 11:41
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI 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.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (4)

imap_processing/hi/hi_l2.py:703

  • combine_maps() currently exposure-averages ena_intensity_sys_err independently of the two component terms (bg_intensity_sys_err and ena_intensity_calibration_sys_err). That breaks the invariant established earlier in the pipeline (ena_intensity_sys_err = sqrt(bg^2 + calib^2)), so the combined map can become internally inconsistent. Recompute ena_intensity_sys_err from the combined component terms instead of averaging it separately.
        for var in (
            "ena_intensity_sys_err",
            "bg_rate",
            "bg_rate_sys_err",
            "bg_intensity_sys_err",

imap_processing/cdf/config/imap_enamaps_l2-rectangular_variable_attrs.yaml:124

  • The new support variable ena_intensity_calibration_sys_err is defined in the common + rectangular ENA-map attribute YAMLs, but there is no corresponding override entry in imap_enamaps_l2-healpix_variable_attrs.yaml. Healpix ENA map generation drops variables without tiling-specific attribute overrides (or will produce incorrect DEPEND pointers), so this variable may be silently dropped from Healpix map CDFs.
ena_intensity_calibration_sys_err:
  DEPEND_1: energy
  DEPEND_2: longitude
  DEPEND_3: latitude
  LABL_PTR_1: energy_label
  LABL_PTR_2: longitude_label
  LABL_PTR_3: latitude_label

imap_processing/hi/hi_l2.py:464

  • The PR description / linked issue describes adding ena_intensity_background_systematic_err, but the implementation and tests expose this as bg_intensity_sys_err. If downstream consumers (or requirements) expect the ena_intensity_* naming, either rename the output variable (and update the CDF attribute configs accordingly) or update the PR description/issue expectations to match the chosen bg_intensity_sys_err name.
    map_ds : xarray.Dataset
        Map dataset with new variables: ena_intensity, ena_intensity_stat_uncert,
        bg_intensity_sys_err.

imap_processing/hi/hi_l2.py:516

  • flux_correction_ratio is computed as corrected/uncorrected intensity. If pre_correction_intensity is 0 (e.g., a pixel clipped to zero after background subtraction) this produces 0/0 -> NaN, which then turns bg_intensity_sys_err into NaN even though the flux correction itself didn’t introduce missing data. Consider normalizing non-finite ratios to 1.0 to avoid poisoning the systematic-error field.
        with np.errstate(divide="ignore", invalid="ignore"):
            flux_correction_ratio = map_ds["ena_intensity"] / pre_correction_intensity
        map_ds["bg_intensity_sys_err"] = (
            map_ds["bg_intensity_sys_err"] * flux_correction_ratio
        )

@tech3371 tech3371 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.

The code changes looks great to me. comments made it easy to follow updates without knowing depth. Thank you!

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.

looks like lot of math update happening. Comments were helpful as reader!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Hi L2 - Add L2 Map variables

3 participants