dascore/utils/pd.py (1)

453-457: Docstring over‑specifies dtype; align wording with implementation.

The fallback returns a numpy array broadcast of the provided value (not necessarily bool). Suggest clarifying the docstring.

-    """Return the values from a column, if they exist, else bool array of value."""
+    """Return column values if present; else a numpy array broadcast of `value` to len(df)."""

dascore/core/spool.py (2)

521-523: Correct: chunk on source_df to prevent “double chunk”. Remove stale comment.

Using self._source_df here fixes #533 semantics. Drop the commented legacy line.

-        df = self._source_df.drop(columns=list(self._drop_columns), errors="ignore")
-        # df = self._df
+        df = self._source_df.drop(columns=list(self._drop_columns), errors="ignore")

---

574-582: Select: align source view with instruction indices.

Adjusting source via adjust_segments(self._source_df.loc[inst.index], …) eliminates desync between current/instructions/source during chained ops. Good fix.

Tiny cleanup: the “Determine if the instructions are the same as the source dataframe.” comment above no longer reflects the logic; consider removing.

tests/test_core/test_patch_chunk.py (3)

502-536: Solid regression for #533; reduce order‑coupling to DataFrame rows.

Accessing result_contents.iloc[i] assumes DataFrame row order matches iteration order. Safer to reset the index to guarantee alignment.

-        result_contents = result_spool.get_contents()
+        result_contents = result_spool.get_contents().reset_index(drop=True)

---

537-548: Make the example explicit to avoid fixture drift.

Use get_example_spool("random_das") for determinism if defaults ever change.

-        spool = dc.get_example_spool()
+        spool = dc.get_example_spool("random_das")

---

549-579: Good chain test; mirror the index reset as above.

Same order assumption here; apply reset_index for robustness.

-        result_contents = result_spool.get_contents()
+        result_contents = result_spool.get_contents().reset_index(drop=True)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

tests/test_core/test_patch_chunk.py (1)

561-576: Repeat: avoid assuming dataframe row order matches iteration order.

Mirror the vectorized checks from the earlier test to reduce brittleness.

Apply this diff:

-        result_contents = result_spool.get_contents().reset_index(drop=True)
-        for i, patch in enumerate(result_spool):
-            # Assert no NaN values in patch attributes
-            assert not pd.isna(patch.attrs["time_min"]), f"Patch {i} has NaN time_min"
-            assert not pd.isna(patch.attrs["time_max"]), f"Patch {i} has NaN time_max"
-
-            # Verify dataframe contains reasonable time values
-            df_row = result_contents.iloc[i]
-            df_time_min = dc.to_datetime64(df_row["time_min"])
-            df_time_max = dc.to_datetime64(df_row["time_max"])
-
-            # Dataframe times should not be NaN or invalid
-            assert not pd.isna(df_time_min), f"DF row {i} has NaN time_min"
-            assert not pd.isna(df_time_max), f"DF row {i} has NaN time_max"
-            assert df_time_min <= df_time_max, f"DF row {i} has invalid time range"
+        result_contents = result_spool.get_contents()
+        for i, patch in enumerate(result_spool):
+            assert not pd.isna(patch.attrs["time_min"]), f"Patch {i} has NaN time_min"
+            assert not pd.isna(patch.attrs["time_max"]), f"Patch {i} has NaN time_max"
+        assert result_contents["time_min"].notna().all()
+        assert result_contents["time_max"].notna().all()
+        dt_min = dc.to_datetime64(result_contents["time_min"])
+        dt_max = dc.to_datetime64(result_contents["time_max"])
+        assert (dt_min <= dt_max).all()

Optional: extract a small helper to DRY these assertions across tests.

def _assert_df_times_valid(spool):
    df = spool.get_contents()
    assert df["time_min"].notna().all()
    assert df["time_max"].notna().all()
    dt_min = dc.to_datetime64(df["time_min"])
    dt_max = dc.to_datetime64(df["time_max"])
    assert (dt_min <= dt_max).all()

tests/test_core/test_patch_chunk.py (4)

517-519: Relax assertion to avoid data-dependent flakiness.

Requiring more patches than the original spool is brittle if the merged duration is short. Prefer asserting non-emptiness and/or relative to the merged count.

Apply this diff:

-        # Should have more patches (chunking into smaller pieces)
-        assert len(result_spool) > len(spool)
+        # Result should be non-empty and not smaller than the merged count.
+        assert len(result_spool) >= 1
+        assert len(result_spool) >= len(chunk_1_spool)

---

521-536: Decouple patch iteration from dataframe row order to prevent order-coupling flakes.

Indexing result_contents.iloc[i] assumes iteration order matches dataframe row order; that isn’t guaranteed. Validate patch attrs and dataframe columns independently.

Apply this diff:

-        result_contents = result_spool.get_contents().reset_index(drop=True)
-        for i, patch in enumerate(result_spool):
-            # Assert no NaN values in patch attributes
-            assert not pd.isna(patch.attrs["time_min"]), f"Patch {i} has NaN time_min"
-            assert not pd.isna(patch.attrs["time_max"]), f"Patch {i} has NaN time_max"
-
-            # Verify dataframe contains reasonable time values
-            df_row = result_contents.iloc[i]
-            df_time_min = dc.to_datetime64(df_row["time_min"])
-            df_time_max = dc.to_datetime64(df_row["time_max"])
-
-            # Dataframe times should not be NaN or invalid
-            assert not pd.isna(df_time_min), f"DF row {i} has NaN time_min"
-            assert not pd.isna(df_time_max), f"DF row {i} has NaN time_max"
-            assert df_time_min <= df_time_max, f"DF row {i} has invalid time range"
+        result_contents = result_spool.get_contents()
+        # Assert patch attributes are valid
+        for i, patch in enumerate(result_spool):
+            assert not pd.isna(patch.attrs["time_min"]), f"Patch {i} has NaN time_min"
+            assert not pd.isna(patch.attrs["time_max"]), f"Patch {i} has NaN time_max"
+        # Assert dataframe consistency without assuming row order == iteration order
+        assert result_contents["time_min"].notna().all()
+        assert result_contents["time_max"].notna().all()
+        dt_min = dc.to_datetime64(result_contents["time_min"])
+        dt_max = dc.to_datetime64(result_contents["time_max"])
+        assert (dt_min <= dt_max).all()

---

540-546: Strengthen the chained-chunk test with a basic non-emptiness check.

Quick sanity guard before iterating.

Apply this diff:

-        # Should be able to access all patches
+        # Should be able to access all patches
+        assert len(result_spool) > 0
         for i in range(len(result_spool)):
             patch = result_spool[i]
             assert isinstance(patch, dc.Patch)

---

549-552: Assert equivalence to direct merge to validate end-to-end idempotence.

Merging after splitting should equal a direct merge on the same input (for consistent attrs). Adds a stronger check for #533.

Apply this diff:

         # First chunk into smaller pieces, then merge back
         result_spool = random_spool.chunk(time=1).chunk(time=...)
 
+        # Should match a direct merge from the original spool
+        expected_spool = random_spool.chunk(time=...)
+        assert result_spool == expected_spool

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro