Skip to content
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.

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

Many functions on normalized FoldedLightCurve failed with UnitConversionError: copy(), select_flux(), normalize(), etc. #1429

Open
orionlee opened this issue May 13, 2024 · 1 comment
Labels
🐛 bug Something isn't working

Comments

@orionlee
Copy link
Collaborator

orionlee commented May 13, 2024

Problem description

Many operations on a normalized FoldedLighCurve object, created from lc.fold(..., normalize_phase=True), would fail with UnitConversionError: only quantities with time units can be used to instantiate Time instances.

The affected functions all use all use lc.copy() internally, which triggered the error. They include (not exhaustive): lc.copy(), lc.select_flux(), lc.remove_outliers(), lc.normalize(), lightcurve math (e.g., lc * 1.1), etc.

Some operations are not affected, including lc.remove_nans() , lc.truncate() (operations that create a new lighcurve object using numpy boolean indexing)

Example

import lightkurve as lk
# insert code here ...
lc = lk.search_lightcurve("TIC261136679", mission="TESS", cadence="short")[-1].download()  # pi Men

lc_f = lc.fold(epoch_time=1325.5, period=6.26782, normalize_phase=True)  

# They all fail with UnitConversionError, caused by `self.copy()` in implementation
lc_f.copy()
lc_f.select_flux("sap_flux") 
lc_f.remove_outliers()
lc_f * 1.1
lc_f.normalize()

The stacktrace:

UnitConversionError                       Traceback (most recent call last)
File C:\pkg\_winNonPortables\miniforge3\envs\lk_dev\lib\site-packages\astropy\time\utils.py:105, in quantity_day_frac(val1, val2)
    104 try:
--> 105     factor = val1.unit.to(u.day)
    106 except Exception:
    107     # Not a simple scaling, so cannot do the full-precision one.
    108     # But at least try normal conversion, since equivalencies may be set.

File C:\pkg\_winNonPortables\miniforge3\envs\lk_dev\lib\site-packages\astropy\units\core.py:1165, in UnitBase.to(self, other, value, equivalencies)
   1164 else:
-> 1165     return self._get_converter(Unit(other), equivalencies)(value)

File C:\pkg\_winNonPortables\miniforge3\envs\lk_dev\lib\site-packages\astropy\units\core.py:1094, in UnitBase._get_converter(self, other, equivalencies)
   1092                 return lambda v: b(converter(v))
-> 1094 raise exc

File C:\pkg\_winNonPortables\miniforge3\envs\lk_dev\lib\site-packages\astropy\units\core.py:1077, in UnitBase._get_converter(self, other, equivalencies)
   1076 try:
-> 1077     return self._apply_equivalencies(
   1078         self, other, self._normalize_equivalencies(equivalencies)
   1079     )
   1080 except UnitsError as exc:
   1081     # Last hope: maybe other knows how to do it?
   1082     # We assume the equivalencies have the unit itself as first item.
   1083     # TODO: maybe better for other to have a `_back_converter` method?

File C:\pkg\_winNonPortables\miniforge3\envs\lk_dev\lib\site-packages\astropy\units\core.py:1054, in UnitBase._apply_equivalencies(self, unit, other, equivalencies)
   1052 other_str = get_err_str(other)
-> 1054 raise UnitConversionError(f"{unit_str} and {other_str} are not convertible")

UnitConversionError: '' (dimensionless) and 'd' (time) are not convertible

During handling of the above exception, another exception occurred:

UnitConversionError                       Traceback (most recent call last)
File C:\pkg\_winNonPortables\miniforge3\envs\lk_dev\lib\site-packages\astropy\units\quantity.py:959, in Quantity.to_value(self, unit, equivalencies)
    958 try:
--> 959     scale = self.unit._to(unit)
    960 except Exception:
    961     # Short-cut failed; try default (maybe equivalencies help).
...
...
UnitConversionError                       Traceback (most recent call last)
Cell In[25], line 6
      3 lc_f = lc.fold(epoch_time=1325.5, period=6.26782, normalize_phase=True)  
      5 # They all fail caused by `self.copy()` in implementation
----> 6 lc_f.copy()
      7 lc_f.select_flux("flux") 
      8 lc_f.remove_outliers()

File C:\pkg\_winNonPortables\miniforge3\envs\lk_dev\lib\site-packages\astropy\table\table.py:3664, in Table.copy(self, copy_data)
   3653 def copy(self, copy_data=True):
   3654     """
   3655     Return a copy of the table.
   3656 
   (...)
   3662         deepcopied regardless of the value for ``copy_data``.
   3663     """
-> 3664     out = self.__class__(self, copy=copy_data)
   3666     # If the current table is grouped then do the same in the copy
   3667     if hasattr(self, "_groups"):

File C:\dev\lightkurve\src\lightkurve\lightcurve.py:319, in LightCurve.__init__(self, data, time, flux, flux_err, *args, **kwargs)
    317 if data is not None and has_time_in_data():
    318     if not isinstance(get_time_in_data(), (Time, TimeDelta)):
--> 319         tmp_time = Time(
    320             get_time_in_data(),
    321             format=deprecated_kws.get("time_format", self._default_time_format),
    322             scale=deprecated_kws.get("time_scale", self._default_time_scale),
    323         )
    324         if _is_np_structured_array(data):
    325             # special case for np structured array
    326             # one cannot set a `Time` instance to it
    327             # so we set the time to the `time` param, and take it out of data
    328             time = tmp_time

File C:\pkg\_winNonPortables\miniforge3\envs\lk_dev\lib\site-packages\astropy\time\core.py:1853, in Time.__init__(self, val, val2, format, scale, precision, in_subfmt, out_subfmt, location, copy)
   1851         self._set_scale(scale)
   1852 else:
-> 1853     self._init_from_vals(
   1854         val, val2, format, scale, copy, precision, in_subfmt, out_subfmt
   1855     )
   1856     self.SCALES = TIME_TYPES[self.scale]
   1858 if self.location is not None and (
   1859     self.location.size > 1 and self.location.shape != self.shape
   1860 ):

File C:\pkg\_winNonPortables\miniforge3\envs\lk_dev\lib\site-packages\astropy\time\core.py:536, in TimeBase._init_from_vals(self, val, val2, format, scale, copy, precision, in_subfmt, out_subfmt)
    533 mask, val, val2 = _check_for_masked_and_fill(val, val2)
    535 # Parse / convert input values into internal jd1, jd2 based on format
--> 536 self._time = self._get_time_fmt(
    537     val, val2, format, scale, precision, in_subfmt, out_subfmt
    538 )
    539 self._format = self._time.name
    541 # Hack from #9969 to allow passing the location value that has been
    542 # collected by the TimeAstropyTime format class up to the Time level.
    543 # TODO: find a nicer way.

File C:\pkg\_winNonPortables\miniforge3\envs\lk_dev\lib\site-packages\astropy\time\core.py:599, in TimeBase._get_time_fmt(self, val, val2, format, scale, precision, in_subfmt, out_subfmt)
    597 for name, cls in formats:
    598     try:
--> 599         return cls(val, val2, scale, precision, in_subfmt, out_subfmt)
    600     except UnitConversionError:
    601         raise

File C:\pkg\_winNonPortables\miniforge3\envs\lk_dev\lib\site-packages\astropy\time\formats.py:152, in TimeFormat.__init__(self, val1, val2, scale, precision, in_subfmt, out_subfmt, from_jd)
    150     self.jd2 = val2
    151 else:
--> 152     val1, val2 = self._check_val_type(val1, val2)
    153     self.set_jds(val1, val2)

File C:\pkg\_winNonPortables\miniforge3\envs\lk_dev\lib\site-packages\astropy\time\formats.py:476, in TimeNumeric._check_val_type(self, val1, val2)
    473 orig_val2_is_none = val2 is None
    475 if val1.dtype.kind == "f":
--> 476     val1, val2 = super()._check_val_type(val1, val2)
    477 elif not orig_val2_is_none or not (
    478     val1.dtype.kind in "US"
    479     or (
   (...)
    482     )
    483 ):
    484     raise TypeError(
    485         f"for {self.name} class, input should be doubles, string, or Decimal, "
    486         "and second values are only allowed for doubles."
    487     )

File C:\pkg\_winNonPortables\miniforge3\envs\lk_dev\lib\site-packages\astropy\time\formats.py:326, in TimeFormat._check_val_type(self, val1, val2)
    324     val1, val2 = quantity_day_frac(val1, val2)
    325 except u.UnitsError:
--> 326     raise u.UnitConversionError(
    327         "only quantities with time units can be "
    328         "used to instantiate Time instances."
    329     )
    330 # We now have days, but the format may expect another unit.
    331 # On purpose, multiply with 1./day_unit because typically it is
    332 # 1./erfa.DAYSEC, and inverting it recovers the integer.
    333 # (This conversion will get undone in format's set_jds, hence
    334 # there may be room for optimizing this.)
    335 factor = 1.0 / getattr(self, "unit", 1.0)

UnitConversionError: only quantities with time units can be used to instantiate Time instances.

Expected behavior

The operations should work on normalized FoldedLightCurve.

Environment

  • lightkurve version (e.g. 1.0b6): 2.4.2
@orionlee
Copy link
Collaborator Author

The problem is similar to #1422 . While the specific code paths are different, the nature of underlying issue is the same: implementation assumes lc.time is Time / TimeDelta, but for normalized FoldedLightCurve, lc.time is Quantity with unit u.dimensionless_unscaled (conceptually floats).

@orionlee orionlee added the 🐛 bug Something isn't working label May 13, 2024
@orionlee orionlee changed the title Many functions on normalized FoldedLightCurve failed with UnitConversionErrorL copy(), select_flux(), normalize(), etc. Many functions on normalized FoldedLightCurve failed with UnitConversionError: copy(), select_flux(), normalize(), etc. May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant