Skip to content

Fix GLOWS L2 - add per-bin zero handling for flux calculations #2890

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vmartinez-cu wants to merge 2 commits into
base: dev
Choose a base branch
from
+28 −2
Open

Fix GLOWS L2 - add per-bin zero handling for flux calculations #2890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c438107
Check that exposure times are not zero to prevent division by zero wh…
vmartinez-cu Mar 30, 2026
7c19097
Address PR comments - add check that all exposure times are the same …
vmartinez-cu Mar 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions imap_processing/glows/l2/glows_l2_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,11 @@ def __post_init__(self, l1b_data: xr.Dataset, position_angle: float) -> None:
self.photon_flux = np.zeros(self.number_of_bins)
self.flux_uncertainties = np.zeros(self.number_of_bins)

# TODO: Only where exposure counts != 0
if len(self.exposure_times) != 0:
if (
len(self.exposure_times) != 0
and self.exposure_times[0] > 0
and len(np.unique(self.exposure_times)) == 1
):
Comment on lines +109 to +113
Copy link
Copy Markdown
Collaborator Author

@vmartinez-cu vmartinez-cu Mar 31, 2026

Choose a reason for hiding this comment

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

@maxinelasp What should happen if these conditions aren't met. Should an error be raised or should processing proceed?

self.photon_flux = self.raw_histograms / self.exposure_times
self.flux_uncertainties = raw_uncertainties / self.exposure_times

Expand Down
23 changes: 23 additions & 0 deletions imap_processing/tests/glows/test_glows_l2_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,29 @@ def test_zero_exposure_bins(l1b_dataset, mock_ecliptic_bin_centers):
assert np.allclose(lc.exposure_times, expected_exposure)


def test_zero_exposure_values(l1b_dataset, mock_ecliptic_bin_centers):
"""Zero exposure yields zero flux and zero uncertainty per bin."""

# Note: all bins have the same exposure time, so if one is zero all are zero.

# Update values used to calculate exposure times to
# ensure a zero exposure result.
l1b_dataset["spin_period_average"].data[:] = 0
l1b_dataset["number_of_spins_per_block"].data[:] = 0

with np.errstate(divide="raise", invalid="raise"):
lc = DailyLightcurve(l1b_dataset, position_angle=0.0)

expected = np.zeros(l1b_dataset.sizes["bins"], dtype=float)
assert lc.exposure_times.shape == expected.shape
assert len(np.unique(lc.exposure_times)) == 1
assert np.array_equal(lc.exposure_times, expected)
assert np.array_equal(lc.photon_flux, expected)
assert np.array_equal(lc.flux_uncertainties, expected)
assert np.all(np.isfinite(lc.photon_flux))
assert np.all(np.isfinite(lc.flux_uncertainties))


Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

This test only covers the case where all exposure values are zero. Since the underlying bug report/PR title calls for per-bin zero handling, add a test case that exercises mixed exposure values (some bins zero, others non-zero) once the implementation supports it, to ensure only the zero-exposure bins produce zero flux/uncertainty and no divide-by-zero occurs.

Suggested change
@pytest.mark.xfail(
reason=(
"Per-bin zero exposure handling not yet implemented. "
"Once supported, construct a dataset with mixed exposure values "
"(some bins zero, others non-zero) and assert that only the "
"zero-exposure bins have zero flux/uncertainty with no divide-by-zero."
)
)
def test_mixed_zero_exposure_values(l1b_dataset, mock_ecliptic_bin_centers):
"""Placeholder for mixed per-bin exposure test.
This test should be updated once the pipeline supports per-bin zero
exposure handling. The intended behavior is:
* Bins with zero exposure time produce zero photon_flux and
flux_uncertainties.
* Bins with non-zero exposure time retain finite, non-NaN values.
* No divide-by-zero or invalid floating-point operations occur even when
using np.errstate(divide="raise", invalid="raise").
"""
pytest.xfail(
"TODO: implement mixed per-bin exposure test when exposure can vary by bin."
)

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

@vmartinez-cu vmartinez-cu Mar 31, 2026

Choose a reason for hiding this comment

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

All bins have the same exposure time so if one is zero all will be zero

def test_number_of_bins(l1b_dataset, mock_ecliptic_bin_centers):
lc = DailyLightcurve(l1b_dataset, position_angle=0.0)
assert lc.number_of_bins == 4
Expand Down
Loading