Repository: lightkurve/lightkurve

Issue:
`LightCurve.select_flux()` would cause inconsistent units between flux and flux_err in some cases, e.g., `flux_bkg` from GSFC-ELEANOR-LITE
<!-- Fill in the information below before opening an issue. -->

#### Problem description
<!-- Provide a clear and concise description of the issue. -->

`LightCurve.select_flux()` would cause inconsistent units between flux and flux_err in some cases, e.g., `flux_bkg` from GSFC-ELEANOR-LITE. The inconsistency would lead to errors in some usage.


#### Example
<!-- Provide a link or minimal code snippet that demonstrates the issue. -->
```python
import lightkurve as lk

lc = lk.search_lightcurve("TIC 336732616", author="GSFC-ELEANOR-LITE", sector=1)[0].download()
print("Units of flux, flux_err:", lc.flux.unit, ",", lc.flux_err.unit, "; origin:", lc.flux_origin);

lc = lc.select_flux("flux_bkg");
print("Units of flux, flux_err:", lc.flux.unit, ",", lc.flux_err.unit, "; origin:", lc.flux_origin);

lc = lc["time", "flux", "flux_err"]  # ValueError
```

The output is:

```python
Units of flux, flux_err: electron / s , electron / s ; origin: corr_flux
Units of flux, flux_err: % , electron / s ; origin: flux_bkg
```

with a `ValueError`
```python
ValueError                                Traceback (most recent call last)
Cell In[1178], line 7
      4 lc = lc.select_flux("flux_bkg");
      5 print("Units of flux, flux_err:", lc.flux.unit, ",", lc.flux_err.unit, "; origin:", lc.flux_origin);
----> 7 lc = lc["time", "flux", "flux_err"]

File C:\pkg\_winNonPortables\miniforge3\envs\my_lk_plus\lib\site-packages\astropy\timeseries\sampled.py:293, in TimeSeries.__getitem__(self, item)
    289         out._groups = groups.TableGroups(
    290             out, indices=self.groups._indices, keys=self.groups._keys
    291         )
    292         return out
--> 293 return super().__getitem__(item)

File C:\pkg\_winNonPortables\miniforge3\envs\my_lk_plus\lib\site-packages\astropy\table\table.py:2070, in Table.__getitem__(self, item)
   2068     return self.Row(self, item.item())
   2069 elif self._is_list_or_tuple_of_str(item):
-> 2070     out = self.__class__(
   2071         [self[x] for x in item], copy_indices=self._copy_indices
   2072     )
   2073     out._groups = groups.TableGroups(
   2074         out, indices=self.groups._indices, keys=self.groups._keys
   2075     )
   2076     out.meta = self.meta.copy()  # Shallow copy for meta

File C:\pkg\_winNonPortables\miniforge3\envs\my_lk_plus\lib\site-packages\lightkurve\lightcurve.py:400, in LightCurve.__init__(self, data, time, flux, flux_err, *args, **kwargs)
    398 # Ensure flux and flux_err have the same units
    399 if self["flux"].unit != self["flux_err"].unit:
--> 400     raise ValueError("flux and flux_err must have the same units")
    402 self._new_attributes_relax = False
    403 self._required_columns_relax = False

ValueError: flux and flux_err must have the same units
```

#### Expected behavior
<!-- Describe the behavior you expected and how it differs from the behavior observed in the example. -->
`select_flux()` should not cause inconsistency between flux / flux_err.

#### Environment

-  platform (e.g. Linux, OSX, Windows): Windows
-  lightkurve version (e.g. 1.0b6): 2.5.0
-  installation method (e.g. pip, conda, source): conda



Discussion:
orionlee: Also, an edge case this PR currently does not address is that if the new flux has associated error column but they have different units, e.g, column `flux_bkg` in % and column `flux_bkg_err` in e-/s, the lc returned from `.select_flux("flux_bkg")` will have inconsistent units.

We might consider to issue a warning for such scenarios.
Nschanche: > Also, an edge case this PR currently does not address is that if the new flux has associated error column but they have different units, e.g, column `flux_bkg` in % and column `flux_bkg_err` in e-/s, the lc returned from `.select_flux("flux_bkg")` will have inconsistent units.
> 
> We might consider to issue a warning for such scenarios.

The only scenario where I think it makes sense to only have a warning is the one you mention, with a mix between a percentage and a unit. However, I think users are more likely to ignore the warning and treat it as though they are the same unit.  I think it is safer and more consistent if it breaks if there is a mismatch.
orionlee: > I think it is safer and more consistent if it breaks if there is a mismatch.

Do you mean that `.select_flux()` should raise an exception for cases like this? If users wants to proceed, they'd need to first fix the inconsistency in the underlying columns before calling `.select_flux()`.

It's a slight change in API behavior, but that's probably okay, given it's a really edge case. 
I can bundle the change in this PR if there's an agreement in the behavior.

Nschanche: > > I think it is safer and more consistent if it breaks if there is a mismatch.
> 
> Do you mean that `.select_flux()` should raise an exception for cases like this? If users wants to proceed, they'd need to first fix the inconsistency in the underlying columns before calling `.select_flux()`.
> 
> It's a slight change in API behavior, but that's probably okay, given it's a really edge case. I can bundle the change in this PR if there's an agreement in the behavior.

That's what I think. @tylerapritchard, do you want to weigh in?
rebekah9969: @Nschanche and @orionlee  - I agree that the code should break if there is a mismatch and so force the user to address that the units are not the same and fix that before moving forward. If we fix it for them i worry they would continue to miss the issue and make other assumptions. 
orionlee: @rebekah9969 To clarify (in case there is confusion), fixing the unit mismatch is never on the table in the first place. There are two cases:

1. (the base case) current `flux` / `flux_err` is in e-/s, while `new_flux` is in %, but there is no `new_flux_err`  (such as flux_bkg from GSFC-ELEANOR-LITE).
The current `lc.select_flux()` will result in the new `flux` in % while, the all-nan `flux_err` (filled by `lightkurve`) in e-/s  . The PR fixes the mismatch created by lightkurve itself.

2. (the really edge, hypothetical, case) current `flux` / `flux_err` is in e-/s, while `new_flux` is in %, but `new_flux_err` is in a different unit, say, e-/s.
The current `lc.select_flux()` will result in the new `flux` in % and `flux_err` in e-/s, without checking the units are inconsistent. The proposal is to raise an error instead, to force users address the mismatch.

Nschanche: > @rebekah9969 To clarify (in case there is confusion), fixing the unit mismatch is never on the table in the first place. There are two cases:
> 
> 1. (the base case) current `flux` / `flux_err` is in e-/s, while `new_flux` is in %, but there is no `new_flux_err`  (such as `flux_bkg` from GSFC-ELEANOR-LITE).
>    The current `lc.select_flux()` will result in the new `flux` in % while, the all-nan `flux_err` (filled by `lightkurve`) in e-/s  . The PR fixes the mismatch created by lightkurve itself.
> 2. (the really edge, hypothetical, case) current `flux` / `flux_err` is in e-/s, while `new_flux` is in %, but `new_flux_err` is in a different unit, say, e-/s.
>    The current `lc.select_flux()` will result in the new `flux` in % and `flux_err` in e-/s, without checking the units are inconsistent. The proposal is to raise an error instead, to force users address the mismatch.

I think that we do want to throw the error from option 2 in this unlikely event. 
orionlee: The codes now raise a `ValueError` if the flux and flux_err columns to be selected have different units.

The PR is ready to merge, pending review feedback. Changelog entry to be added is in the first comment.

orionlee: Changelog added.

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index ed1e86b7..b9272dae 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -1,8 +1,12 @@
 2.5.1  (unreleased)
 =====================
-- Fixed pixel to world coordinate transformation in ``TargetPixelFile.get_coordinates()`` 
-  in line 488 ("ra, dec = w.wcs_pix2world(X.ravel(), Y.ravel(), 1)"), where for consistency with 
-  Gaia the origin should be 0 instead of 1.
+
+- Fixed pixel to world coordinate transformation in ``TargetPixelFile.get_coordinates()``
+  in line 488 ("ra, dec = w.wcs_pix2world(X.ravel(), Y.ravel(), 1)"), where for consistency with
+  Gaia the origin should be 0 instead of 1. [#1465]
+- Fixed ``LightCurve.select_flux()`` in edge cases, where the unit of
+  the new ``flux`` column is different from that of the ``flux_err`` column. [#1467]
+
 
 2.5.0 (2024-08-29)
 =====================
diff --git a/src/lightkurve/lightcurve.py b/src/lightkurve/lightcurve.py
index dbc92f84..bc90aa22 100644
--- a/src/lightkurve/lightcurve.py
+++ b/src/lightkurve/lightcurve.py
@@ -559,7 +559,21 @@ class LightCurve(TimeSeries):
             if flux_err_column in lc.columns:
                 lc["flux_err"] = lc[flux_err_column]
             else:
-                lc["flux_err"][:] = np.nan
+                # fill in a dummy all-nan flux_err column
+                # ensure the unit of new flux_err is consistent with that of flux.
+                flux_err_col_vals = np.full(lc["flux"].shape, np.nan)
+                if lc["flux"].unit is not None:
+                    flux_err_col_vals = flux_err_col_vals * lc["flux"].unit
+                lc["flux_err"] = flux_err_col_vals
+
+        # Ensure resulting flux / flux_err have the same
+        # Do the check here after the columns are selected so as to uniformly handle
+        # different cases.
+        if lc["flux"].unit != lc["flux_err"].unit:
+            raise ValueError(
+                f"Columns '{flux_column}' and '{flux_err_column}' have different units: "
+                f"{lc.flux.unit} and {lc.flux_err.unit} respectively."
+            )
 
         lc.meta['FLUX_ORIGIN'] = flux_column
         normalized_new_flux = lc["flux"].unit is None or lc["flux"].unit is u.dimensionless_unscaled


Test patch:
diff --git a/tests/io/test_eleanor.py b/tests/io/test_eleanor.py
index 7a49cb73..3e8126b7 100644
--- a/tests/io/test_eleanor.py
+++ b/tests/io/test_eleanor.py
@@ -3,7 +3,7 @@ import pytest
 from astropy.io import fits
 from astropy.utils.data import get_pkg_data_filename
 import numpy as np
-from numpy.testing import assert_array_equal
+from numpy.testing import assert_array_equal, assert_equal
 
 from lightkurve import search_lightcurve
 from lightkurve.io.eleanor import read_eleanor_lightcurve
@@ -32,6 +32,10 @@ def test_gsfc_eleanor_lite():
         assert not (lc["quality"] & (2**17 | 2**18)).any()
         assert np.issubdtype(lc["cadenceno"].dtype, np.integer)
 
+        # https://github.com/lightkurve/lightkurve/issues/1467
+        lc = lc.select_flux("flux_bkg")
+        assert_equal(lc["flux_err"].unit, lc["flux"].unit)
+
 
 @pytest.mark.parametrize(
     "url",
diff --git a/tests/test_lightcurve.py b/tests/test_lightcurve.py
index f8a0adcf..43222b50 100644
--- a/tests/test_lightcurve.py
+++ b/tests/test_lightcurve.py
@@ -1947,6 +1947,10 @@ def test_select_flux():
                           'newflux_err': [7, 8, 9] * u_e_s,
                           'newflux_n1': [0.9, 1, 1.1] * u.dimensionless_unscaled,  # normalized, unitless
                           'newflux_n2': [0.9, 1, 1.1],  # normalized, no unit
+                          'newflux_n3': [4, 5, 6] * u_e_s,  # case flux and _err have different units
+                          'newflux_n3_err': [1, 2, 3] * u.percent,
+                          'newflux_n4': [4, 5, 6] * u_e_s,  # case flux and _err have different units
+                          'newflux_n4_err': [.01, .02, .03],  # normalized, no unit
                           },
                           )
     # Can we set flux to newflux?
@@ -1960,6 +1964,15 @@ def test_select_flux():
     assert all(lc.select_flux("newflux", flux_err_column="newflux").flux_err == lc.newflux)
     # ensure flux_err in the new lc is nan if the origin does not have it
     assert all(np.isnan(lc.select_flux("newflux_n1")["flux_err"]))
+    assert_equal(  # https://github.com/lightkurve/lightkurve/issues/1467
+        lc.select_flux("newflux_n1")["flux_err"].unit, lc.select_flux("newflux_n1")["flux"].unit,
+        "The unit of the all-nan flux_err should be the same as that of flux [#1467]"
+    )
+    # Do inconsistent units in the selected columns raise a ValueError? [issue 1467]
+    with pytest.raises(ValueError, match="different units"):
+        lc.select_flux("newflux_n3")
+    with pytest.raises(ValueError, match="different units"):
+        lc.select_flux("newflux_n4")
     # Do invalid column names raise a ValueError?
     with pytest.raises(ValueError):
         lc.select_flux("doesnotexist")