Fix GLOWS L2 - add per-bin zero handling for flux calculations by vmartinez-cu · Pull Request #2890 · IMAP-Science-Operations-Center/imap_processing

vmartinez-cu · 2026-03-30T22:01:40Z

This pull request handles zero exposure values in the DailyLightcurve class to prevent division by zero errors and adds a corresponding test to ensure correct behavior.

Closes #2370

Copilot

Pull request overview

This PR updates GLOWS L2 DailyLightcurve flux/uncertainty computation to avoid divide-by-zero when exposure is zero, and adds a regression test around zero-exposure behavior.

Changes:

Adds a guard to skip flux/uncertainty division when exposure is zero.
Adds a unit test ensuring zero exposure produces zero flux/uncertainty without triggering NumPy divide/invalid warnings.

Reviewed changes

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

File	Description
`imap_processing/glows/l2/glows_l2_data.py`	Changes the flux/uncertainty calculation guard to avoid division when exposure is zero.
`imap_processing/tests/glows/test_glows_l2_data.py`	Adds a regression test for all-zero exposure values.

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

Copilot · 2026-03-30T22:04:59Z

+        if len(self.exposure_times) != 0 and self.exposure_times[0] > 0:
            self.photon_flux = self.raw_histograms / self.exposure_times
            self.flux_uncertainties = raw_uncertainties / self.exposure_times


The new guard self.exposure_times[0] > 0 only checks the first element and then divides the entire array. This doesn’t implement per-bin zero handling (as requested in #2370 / PR title) and would still divide by zero if exposure_times ever contains a mix of zeros and non-zeros. Consider using element-wise division (e.g., np.divide(..., out=..., where=self.exposure_times > 0)) so bins with zero exposure remain 0 without relying on the “uniform across bins” assumption.

Suggested change

if len(self.exposure_times) != 0 and self.exposure_times[0] > 0:

self.photon_flux = self.raw_histograms / self.exposure_times

self.flux_uncertainties = raw_uncertainties / self.exposure_times

if self.exposure_times.size > 0:

exposure_mask = self.exposure_times > 0

np.divide(

self.raw_histograms,

self.exposure_times,

out=self.photon_flux,

where=exposure_mask,

)

np.divide(

raw_uncertainties,

self.exposure_times,

out=self.flux_uncertainties,

where=exposure_mask,

)

All bins have the same exposure time so just the first one can be checked

Copilot · 2026-03-30T22:05:00Z

+    assert np.all(np.isfinite(lc.photon_flux))
+    assert np.all(np.isfinite(lc.flux_uncertainties))
+
+


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."

)

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

tech3371

minor requests but looks great. That was so quick! Nice!

maxinelasp

Looks good, thanks Veronica!

vmartinez-cu · 2026-03-31T16:13:51Z

+        if (
+            len(self.exposure_times) != 0
+            and self.exposure_times[0] > 0
+            and len(np.unique(self.exposure_times)) == 1
+        ):


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

…en calculating photon flux and flux uncertainties. Add a unit test for this test case.

…value

…an empty dataset

…Science-Operations-Center#2890) * Check that exposure times are not zero to prevent division by zero when calculating photon flux and flux uncertainties. Add testing for this case. * Address PR comments - check that all exposure times are the same value * Add handling for dataset with zero flux and exposure times to return an empty dataset

vmartinez-cu requested review from Copilot, laspsandoval, maxinelasp and tech3371 March 30, 2026 22:01

Copilot started reviewing on behalf of vmartinez-cu March 30, 2026 22:02 View session

Copilot AI reviewed Mar 30, 2026

View reviewed changes

tech3371 approved these changes Mar 31, 2026

View reviewed changes

Comment thread imap_processing/glows/l2/glows_l2_data.py Outdated

Comment thread imap_processing/tests/glows/test_glows_l2_data.py Outdated

Comment thread imap_processing/glows/l2/glows_l2_data.py Outdated

maxinelasp approved these changes Mar 31, 2026

View reviewed changes

vmartinez-cu commented Mar 31, 2026

View reviewed changes

vmartinez-cu added 4 commits April 1, 2026 09:00

Check that exposure times are not zero to prevent division by zero wh…

84fda36

…en calculating photon flux and flux uncertainties. Add a unit test for this test case.

Address PR comments - add check that all exposure times are the same …

cc88c7a

…value

Add handling for dataset with zero flux and exposure times to return …

b76e88a

…an empty dataset

Edit warning message

39ed28e

vmartinez-cu force-pushed the glows-div-by-zeros branch from 7c19097 to 39ed28e Compare April 1, 2026 15:01

vmartinez-cu merged commit 561aa28 into IMAP-Science-Operations-Center:dev Apr 1, 2026
14 checks passed

vmartinez-cu deleted the glows-div-by-zeros branch April 1, 2026 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Fix GLOWS L2 - add per-bin zero handling for flux calculations#2890
vmartinez-cu merged 4 commits into
IMAP-Science-Operations-Center:devfrom
vmartinez-cu:glows-div-by-zeros

vmartinez-cu commented Mar 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 30, 2026

Uh oh!

vmartinez-cu Mar 31, 2026

Uh oh!

Copilot AI Mar 30, 2026

Uh oh!

vmartinez-cu Mar 31, 2026

Uh oh!

tech3371 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maxinelasp left a comment

Uh oh!

vmartinez-cu Mar 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

-        if len(self.exposure_times) != 0 and self.exposure_times[0] > 0:
-            self.photon_flux = self.raw_histograms / self.exposure_times
-            self.flux_uncertainties = raw_uncertainties / self.exposure_times
+        if self.exposure_times.size > 0:
+            exposure_mask = self.exposure_times > 0
+            np.divide(
+                self.raw_histograms,
+                self.exposure_times,
+                out=self.photon_flux,
+                where=exposure_mask,
+            )
+            np.divide(
+                raw_uncertainties,
+                self.exposure_times,
+                out=self.flux_uncertainties,
+                where=exposure_mask,
+            )

		assert np.all(np.isfinite(lc.photon_flux))
		assert np.all(np.isfinite(lc.flux_uncertainties))

+@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."
+    )

Conversation

vmartinez-cu commented Mar 30, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

vmartinez-cu Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

vmartinez-cu Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

tech3371 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maxinelasp left a comment

Choose a reason for hiding this comment

Uh oh!

vmartinez-cu Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants