change on_deserialize flow

nicHoch · nicHoch · commit a888dcb38a47 · 2026-03-26T09:45:09.000+01:00
diff --git a/stixcore/io/product_processors/fits/processors.py b/stixcore/io/product_processors/fits/processors.py
@@ -823,10 +823,11 @@ def generate_primary_header(self, filename, product, *, version=0):
             if default[0] not in soop_key_names:
                 soop_headers += tuple([default])
 
+        scet_range = product.scet_timerange
         time_headers = (
             # Name, Value, Comment
-            ("OBT_BEG", product.scet_timerange.start.as_float().value, "Start of acquisition time in OBT"),
-            ("OBT_END", product.scet_timerange.end.as_float().value, "End of acquisition time in OBT"),
+            ("OBT_BEG", scet_range.start.as_float().value, "Start of acquisition time in OBT"),
+            ("OBT_END", scet_range.end.as_float().value, "End of acquisition time in OBT"),
             ("TIMESYS", "UTC", "System used for time keywords"),
             ("LEVEL", "L1", "Processing level of the data"),
             ("DATE-OBS", product.utc_timerange.start.fits, "Start of acquisition time in UTC"),
diff --git a/stixcore/products/level3/flarelist.py b/stixcore/products/level3/flarelist.py
@@ -11,7 +11,7 @@
 from stixpy.coordinates.transforms import get_hpc_info
 from stixpy.net.client import STIXClient
 from stixpy.product import Product as STIXPYProduct
-from sunpy.coordinates import HeliographicStonyhurst, Helioprojective, SphericalScreen
+from sunpy.coordinates import HeliographicStonyhurst, Helioprojective
 from sunpy.map import make_fitswcs_header
 from sunpy.net import attrs as a
 from sunpy.time import TimeRange
@@ -77,7 +77,21 @@ def make_stix_fitswcs_header(data, flare_position, *, scale, exposure, rotation_
     return header
 
 
-class FlarePositionMixin:
+class _SerializeMixin:
+    """No-op chain terminator for on_serialize/on_deserialize.
+
+    Functional mixins inherit from this so super() calls always land safely
+    instead of hitting object and raising AttributeError.
+    """
+
+    def on_serialize(self, data):
+        pass
+
+    def on_deserialize(self, data, **kwargs):
+        pass
+
+
+class FlarePositionMixin(_SerializeMixin):
     """_summary_"""
 
     @classmethod
@@ -101,8 +115,8 @@ def add_flare_position(
         data["cpd_path"] = Column(" " * 500, dtype=str, description="TDB")
         data["_position_status"] = Column(False, dtype=bool, description="TDB")
         data["_position_message"] = Column(" " * 500, dtype=str, description="TDB")
-        tx_list, ty_list = [], []
-        solo_x_list, solo_y_list, solo_z_list, peak_time_list = [], [], [], []
+        # tx_list, ty_list = [], []
+        solo_cartesian_list = []
 
         to_remove = []
         pass_filter = 0
@@ -118,6 +132,7 @@ def add_flare_position(
             peak_time = row[peak_time_colname]
             start_time = row[start_time_colname]
             end_time = row[end_time_colname]
+            logger.info(f"Processing flare {i}/{len(data)} at time {start_time} : {end_time} (peak at {peak_time})")
             if filter_function(row):  # and i < 60:
                 pass_filter += 1
                 day = peak_time.to_datetime().date()
@@ -137,6 +152,8 @@ def add_flare_position(
                     logger.warning(f"No ephemeris data found for flare at time {start_time} : {end_time}")
                     data[i]["_position_message"] = "no ephemeris data found"
                     no_ephemeris += 1
+                    solo_cartesian_list.append((np.nan * u.km, np.nan * u.km, np.nan * u.km))
+
                     continue
                 data[i]["anc_ephemeris_path"] = anc_res["path"][0]
 
@@ -153,6 +170,8 @@ def add_flare_position(
                     logger.warning(f"No CPD data found for flare at time {start_time} : {end_time}")
                     data[i]["_position_message"] = "no CPD data found"
                     no_cpd += 1
+                    solo_cartesian_list.append((np.nan * u.km, np.nan * u.km, np.nan * u.km))
+
                     continue
                 if len(cpd_res) > 1:
                     logger.debug(f"Many CPD data found for flare at time {start_time} : {end_time}")
@@ -234,55 +253,48 @@ def add_flare_position(
 
                     roll, solo_xyz, pointing = get_hpc_info(start_time, end_time)
                     solo = HeliographicStonyhurst(*solo_xyz, obstime=peak_time, representation_type="cartesian")
-                    with SphericalScreen(solo, only_off_disk=True):
-                        center_hpc = coord.transform_to(Helioprojective(observer=solo))
-                        tx_list.append(center_hpc.Tx)
-                        ty_list.append(center_hpc.Ty)
-                    solo_x_list.append(solo.cartesian.x)
-                    solo_y_list.append(solo.cartesian.y)
-                    solo_z_list.append(solo.cartesian.z)
-                    peak_time_list.append(peak_time)
+                    # with SphericalScreen(solo, only_off_disk=True):
+                    #     center_hpc = coord.transform_to(Helioprojective(observer=solo))
+                    #     tx_list.append(center_hpc.Tx)
+                    #     ty_list.append(center_hpc.Ty)
+                    solo_cartesian_list.append((solo.cartesian.x, solo.cartesian.y, solo.cartesian.z))
 
                     data[i]["_position_status"] = True
                     data[i]["_position_message"] = "OK"
                 except Exception as e:
                     data[i]["_position_status"] = False
                     data[i]["_position_message"] = f"Error: {type(e)}"
                     logger.warning(f"Error calculating flare position for flare at time {start_time} : {end_time}: {e}")
-                    tx_list.append(np.nan * u.arcsec)
-                    ty_list.append(np.nan * u.arcsec)
-                    solo_x_list.append(np.nan * u.km)
-                    solo_y_list.append(np.nan * u.km)
-                    solo_z_list.append(np.nan * u.km)
-                    peak_time_list.append(peak_time)
+                    # tx_list.append(np.nan * u.arcsec)
+                    # ty_list.append(np.nan * u.arcsec)
+                    solo_cartesian_list.append((np.nan * u.km, np.nan * u.km, np.nan * u.km))
+
             else:
                 to_remove.append(i)
-                tx_list.append(np.nan * u.arcsec)
-                ty_list.append(np.nan * u.arcsec)
-                solo_x_list.append(np.nan * u.km)
-                solo_y_list.append(np.nan * u.km)
-                solo_z_list.append(np.nan * u.km)
-                peak_time_list.append(peak_time)
+                # tx_list.append(np.nan * u.arcsec)
+                # ty_list.append(np.nan * u.arcsec)
+                solo_cartesian_list.append((np.nan * u.km, np.nan * u.km, np.nan * u.km))
                 data[i]["_position_status"] = False
                 data[i]["_position_message"] = "flare did not pass the filter function"
 
-        solo_times = Time(peak_time_list)
+        solo_times = Time(data[peak_time_colname])
+        solo_x, solo_y, solo_z = zip(*solo_cartesian_list)
         hgs_coords = SkyCoord(
-            u.Quantity(solo_x_list),
-            u.Quantity(solo_y_list),
-            u.Quantity(solo_z_list),
+            u.Quantity(solo_x),
+            u.Quantity(solo_y),
+            u.Quantity(solo_z),
             frame=HeliographicStonyhurst(obstime=solo_times),
             representation_type="cartesian",
         )
 
-        hp_coords = SkyCoord(
-            u.Quantity(tx_list), u.Quantity(ty_list), frame=Helioprojective(obstime=solo_times, observer=hgs_coords)
-        )
+        # hp_coords = SkyCoord(
+        #     u.Quantity(tx_list), u.Quantity(ty_list), frame=Helioprojective(obstime=solo_times, observer=hgs_coords)
+        # )
 
         data["location_hgs"] = hgs_coords
         # description="Flare location in Heliographic Stonyhurst coordinates"
 
-        data["location_hp"] = hp_coords
+        # data["location_hp"] = hp_coords
         # description="Flare location in Helioprojective coordinates"
 
         if not keep_all_flares:
@@ -297,35 +309,31 @@ def add_flare_position(
         )
 
     def on_serialize(self, data):
-        for col_name in ("location_hgs", "location_hp"):
-            if col_name in data.colnames:
-                icrs = data[col_name].icrs
-                icrs_coord = SkyCoord(icrs.ra, icrs.dec, icrs.distance, frame="icrs")
-                col_idx = data.colnames.index(col_name)
-                data.remove_column(col_name)
-                data.add_column(icrs_coord, name=col_name, index=col_idx)
-        s = super()
-        if hasattr(s, "on_serialize"):
-            s.on_serialize(data)
-
-    def on_deserialize(self, data, *, peak_time_colname=None):
+        logger.info("FlarePositionMixin on_serialize called, transforming location columns to ICRS for serialization")
+
+        if "location_hgs" in data.colnames:
+            icrs = data["location_hgs"].icrs
+            icrs_coord = SkyCoord(icrs.ra, icrs.dec, icrs.distance, frame="icrs")
+            col_idx = data.colnames.index("location_hgs")
+            data.remove_column("location_hgs")
+            data.add_column(icrs_coord, name="location_icrs", index=col_idx)
+        super().on_serialize(data)
+
+    def on_deserialize(self, data, *, peak_time_colname=None, **kwargs):
+        logger.info(
+            "FlarePositionMixin on_deserialize called, transforming location columns back to heliographic coordinates"
+        )
         peak_col = peak_time_colname or self.peak_time_colname
         if peak_col not in data.colnames:
             logger.warning(f"on_deserialize: column '{peak_col}' not found, skipping location transform")
         else:
             obstime = Time(data[peak_col])
-            if "location_hgs" in data.colnames:
-                data["location_hgs"] = data["location_hgs"].transform_to(HeliographicStonyhurst(obstime=obstime))
-            if "location_hp" in data.colnames:
-                data["location_hp"] = data["location_hp"].transform_to(
-                    Helioprojective(obstime=obstime, observer=data["location_hgs"])
-                )
-        s = super()
-        if hasattr(s, "on_deserialize"):
-            s.on_deserialize(data)
+            if "location_icrs" in data.colnames:
+                data["location_hgs"] = data["location_icrs"].transform_to(HeliographicStonyhurst(obstime=obstime))
+        super().on_deserialize(data, **kwargs)
 
 
-class FlareSOOPMixin:
+class FlareSOOPMixin(_SerializeMixin):
     """_summary_"""
 
     @classmethod
@@ -352,6 +360,14 @@ def add_soop(
         data["soop_id"] = Column(soop_id, dtype=str, description="SOOP ID")
         data["soop_type"] = Column(soop_type, dtype=str, description="name of the SOOP campaign")
 
+    # def on_serialize(self, data):
+    #     logger.info("FlareSOOPMixin on_serialize called, but no special handling implemented for SOOP data")
+    #     super().on_serialize(data)
+
+    # def on_deserialize(self, data, **kwargs):
+    #     logger.info("FlareSOOPMixin on_deserialize called, but no special handling implemented for SOOP data")
+    #     super().on_deserialize(data, **kwargs)
+
 
 class FlarePeakPreviewMixin:
     """Mixin class to add peak preview images to flare list products.
diff --git a/stixcore/products/product.py b/stixcore/products/product.py
@@ -613,16 +613,21 @@ def max_exposure(self):
         return 0.0
 
     def on_serialize(self, data):
-        """Hook called before writing data to FITS. Mixins override and chain via super()."""
-        s = super()
-        if hasattr(s, "on_serialize"):
-            s.on_serialize(data)
+        """Hook called before writing data to FITS. Mixins override and chain via super().
 
-    def on_deserialize(self, data):
+        Uses getattr so plain products without functional mixins are safe —
+        the functional mixins (FlarePositionMixin etc.) appear after GenericProduct
+        in the MRO, so pass would stop the chain before reaching them.
+        """
+        serialize = getattr(super(), "on_serialize", None)
+        if serialize is not None:
+            serialize(data)
+
+    def on_deserialize(self, data, **kwargs):
         """Hook called after reading data from FITS. Mixins override and chain via super()."""
-        s = super()
-        if hasattr(s, "on_deserialize"):
-            s.on_deserialize(data)
+        deserialize = getattr(super(), "on_deserialize", None)
+        if deserialize is not None:
+            deserialize(data, **kwargs)
 
     def find_parent_products(self, root):
         """
diff --git a/stixcore/products/tests/test_flarelist.py b/stixcore/products/tests/test_flarelist.py
@@ -2,12 +2,13 @@
 
 import numpy as np
 import pytest
-from sunpy.coordinates import HeliographicStonyhurst, Helioprojective
+from sunpy.coordinates import HeliographicStonyhurst
 
 import astropy.units as u
 from astropy.coordinates import SkyCoord
 from astropy.io import fits
 from astropy.table import QTable
+from astropy.tests.helper import assert_quantity_allclose
 from astropy.time import Time
 
 from stixcore.io.product_processors.fits.processors import FitsL3Processor
@@ -21,32 +22,32 @@
 def flare_data():
     peak_times = Time("2022-01-01T12:00:00") + np.arange(N) * 600 * u.s
 
+    lon = np.linspace(0, 30, N)
+    lat = np.linspace(-5, 5, N)
+    # mark same rows fully NaN so the entire SkyCoord row is invalid
+    lon[2] = lat[2] = np.nan
+    lon[7] = lat[7] = np.nan
+
     hgs_coords = SkyCoord(
-        lon=np.linspace(0, 30, N) * u.deg,
-        lat=np.linspace(-5, 5, N) * u.deg,
+        lon=lon * u.deg,
+        lat=lat * u.deg,
         radius=np.ones(N) * 1.0 * u.AU,
         frame=HeliographicStonyhurst(obstime=peak_times),
     )
 
-    hp_coords = SkyCoord(
-        Tx=np.linspace(-300, 300, N) * u.arcsec,
-        Ty=np.linspace(-200, 200, N) * u.arcsec,
-        frame=Helioprojective(obstime=peak_times, observer=hgs_coords),
-    )
-
     data = QTable()
     data["peak_UTC"] = peak_times
     data["start_UTC"] = peak_times - 60 * u.s
     data["end_UTC"] = peak_times + 60 * u.s
     data["duration"] = np.ones(N) * 120 * u.s
     data["lc_peak"] = np.ones((N, 5)) * u.ct / u.s
     data["location_hgs"] = hgs_coords
-    data["location_hp"] = hp_coords
 
     return data
 
 
-def test_flarelist_sdcloc_location_roundtrip(flare_data, tmp_path):
+@pytest.fixture
+def written_fits(flare_data, tmp_path):
     prod = FlarelistSDCLoc(
         data=flare_data,
         month=date(2022, 1, 1),
@@ -61,26 +62,42 @@ def test_flarelist_sdcloc_location_roundtrip(flare_data, tmp_path):
     header["SSID"] = 3
     header["DATE-BEG"] = "2022-01-01T00:00:00"
     prod.fits_header = header
-
-    # energy/additional_header_keywords are not set for freshly created products
     prod.energy = None
     prod._additional_header_keywords = []
 
+    writer = FitsL3Processor(tmp_path)
+    written = writer.write_fits(prod)
+    assert len(written) == 1
+
+    return prod, written[0]
+
+
+def test_flarelist_sdcloc_location_roundtrip(written_fits):
+    prod, fits_path = written_fits
     orig_hgs_lon = prod.data["location_hgs"].lon.copy()
     orig_hgs_lat = prod.data["location_hgs"].lat.copy()
-    orig_hp_tx = prod.data["location_hp"].Tx.copy()
-    orig_hp_ty = prod.data["location_hp"].Ty.copy()
-
-    # write via FitsL3Processor — calls on_serialize internally, prod.data unchanged
-    writer = FitsL3Processor(tmp_path)
-    written_file_name = writer.write_fits(prod)
-    assert len(written_file_name) == 1
 
     # read back via Product factory — calls on_deserialize internally
-    recovered = Product(written_file_name[0])
+    recovered = Product(fits_path)
 
     assert isinstance(recovered, FlarelistSDCLoc)
-    assert u.allclose(recovered.data["location_hgs"].lon, orig_hgs_lon, atol=1e-6 * u.deg)
-    assert u.allclose(recovered.data["location_hgs"].lat, orig_hgs_lat, atol=1e-6 * u.deg)
-    assert u.allclose(recovered.data["location_hp"].Tx, orig_hp_tx, atol=1e-3 * u.arcsec)
-    assert u.allclose(recovered.data["location_hp"].Ty, orig_hp_ty, atol=1e-3 * u.arcsec)
+    assert_quantity_allclose(recovered.data["location_hgs"].lon, orig_hgs_lon, atol=1e-6 * u.deg, equal_nan=True)
+    assert_quantity_allclose(recovered.data["location_hgs"].lat, orig_hgs_lat, atol=1e-6 * u.deg, equal_nan=True)
+
+
+def test_flarelist_sdcloc_fits_stores_icrs(written_fits):
+    prod, fits_path = written_fits
+    orig_hgs_lon = prod.data["location_hgs"].lon.copy()
+    orig_hgs_lat = prod.data["location_hgs"].lat.copy()
+
+    # read the DATA extension directly — no on_deserialize, raw FITS content
+    raw = QTable.read(fits_path, hdu="DATA", astropy_native=True)
+
+    assert "location_hgs" not in raw.colnames, "HGS column should not be stored in FITS"
+    assert "location_icrs" in raw.colnames, "ICRS column should be present in FITS"
+
+    # manually transform ICRS back to HGS and compare with original
+    obstime = Time(raw["peak_UTC"])
+    hgs = raw["location_icrs"].transform_to(HeliographicStonyhurst(obstime=obstime))
+    assert_quantity_allclose(hgs.lon, orig_hgs_lon, atol=1e-6 * u.deg, equal_nan=True)
+    assert_quantity_allclose(hgs.lat, orig_hgs_lat, atol=1e-6 * u.deg, equal_nan=True)
