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

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

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

imap_processing/glows/l2/glows_l2_data.py

+        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

imap_processing/tests/glows/test_glows_l2_data.py

+    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!

tech3371 · 2026-03-31T15:23:23Z

imap_processing/glows/l2/glows_l2_data.py


-        # TODO: Only where exposure counts != 0
-        if len(self.exposure_times) != 0:
+        if len(self.exposure_times) != 0 and self.exposure_times[0] > 0:


Why do you check only the first one? Just curious

tech3371 · 2026-03-31T15:24:01Z

imap_processing/tests/glows/test_glows_l2_data.py

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


Ah I see. Can you move this note into the actual code. This answers my question above!

tech3371 · 2026-03-31T15:25:44Z

imap_processing/glows/l2/glows_l2_data.py


-        # TODO: Only where exposure counts != 0
-        if len(self.exposure_times) != 0:
+        if len(self.exposure_times) != 0 and self.exposure_times[0] > 0:


now based on below comment, can you add one more check here to make sure that exposure is same for all? Eg. len(np.unique(self.exposure_times)) == 1

Good suggestion!

maxinelasp

Looks good, thanks Veronica!

…value

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

imap_processing/glows/l2/glows_l2_data.py

+        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?

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

c438107

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

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

maxinelasp approved these changes Mar 31, 2026

View reviewed changes

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

7c19097

…value

vmartinez-cu commented Mar 31, 2026

View reviewed changes

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 wants to merge 2 commits intoIMAP-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!

tech3371 Mar 31, 2026

Uh oh!

tech3371 Mar 31, 2026

Uh oh!

tech3371 Mar 31, 2026

Uh oh!

vmartinez-cu Mar 31, 2026

Uh oh!

maxinelasp left a comment

Uh oh!

vmartinez-cu Mar 31, 2026

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!

tech3371 Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

tech3371 Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

tech3371 Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

vmartinez-cu Mar 31, 2026

Choose a reason for hiding this comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants