Implement rename method in DataCatalog and add corresponding tests

Author: dwr-psandhu

AGENTS.md

@@ -79,23 +79,80 @@ def _build_catalog(self):
 
 ## Implementation Gotchas For Subclasses
 
-### Mixed catalogs: math references alongside raw references
+### `name` is the catalog key — never mutate it directly
 
-When a `DataCatalog` contains both raw `DataReference` entries and `MathDataReference` entries (mixed catalog), rows produced by `to_dataframe().reset_index()` will have `NaN` in the `source` column for math references, because derived references have no file source.
+`DataCatalog._references` is an `OrderedDict` keyed by `ref.name`.  If you mutate
+`ref.name` directly the dictionary key becomes stale and every subsequent
+`catalog.get(ref.name)` raises `KeyError`.
 
-**`get_data_reference` must guard against NaN source**
+**Always rename via `catalog.rename(old_name, new_name)`**, which atomically updates
+both the key and `ref.name`:
 
-Raw entries are keyed by their auto-derived name (from `primary_key` values). Math entries are keyed by their `name` alone. The safe pattern:
+```python
+catalog.rename("JER__tf", "JER_tidal_filtered")  # chainable, raises KeyError / ValueError on conflicts
+```
+
+`rename()` does **not** affect `_source_index` — the source (file path) is unchanged.
+
+### `name` vs display label in mixed catalogs
+
+`name` serves two roles that can conflict:
+
+| Role | Raw DSS example | Transform ref example |
+|------|-----------------|----------------------|
+| Catalog lookup key | `"study.dss::/A/JER/EC//15MIN/F/"` | `"JER__tf"` |
+| User-visible display | *ugly — should be hidden* | *clean — should be shown* |
+
+**General rule** (handled automatically by `get_table_column_width_map()`):
+
+When math refs are present the table automatically gains a `name` column so users
+can see what their transform refs are called.  For managers whose raw-ref names
+are human-readable (e.g. `"RSAC075_flow"`) this is sufficient.
+
+**For managers with ugly raw-ref names** (e.g. `DSSDataUIManager` where the raw-ref
+key is `"filename::pathname"`), override `get_data_catalog()` to inject a `label`
+column:
+
+```python
+def get_data_catalog(self):
+    df = self._dvue_catalog.to_dataframe().reset_index()
+    # blank for raw refs (identified by domain columns); catalog key for derived refs
+    df["label"] = df.apply(
+        lambda r: r["name"] if r.get("ref_type", "raw") != "raw" else "",
+        axis=1,
+    )
+    return df
+```
+
+Add `"label"` to `_get_table_column_width_map()` so it appears first in the table.
+When a `label` column is present the framework suppresses the generic `name`
+injection, so the two columns never double-up.
+
+### `get_data_reference` — always use `row["name"]`
+
+The safest lookup pattern works for **both** raw and math refs:
 
 ```python
 def get_data_reference(self, row):
-    source = row.get("source", None)
-    if pd.isna(source):
-        return self._dvue_catalog.get(row["name"])  # math ref
-    return self._dvue_catalog.get(row["name"])       # raw ref — name is always reliable
+    if "name" in row.index:
+        return self._dvue_catalog.get(row["name"])
+    return self._dvue_catalog.get(self.build_ref_key(row))  # fallback without reset_index()
 ```
 
-Failing to guard against NaN produces a `KeyError: "No DataReference named 'nan::...' in catalog."`.
+**Never** branch on `pd.isna(row["filename"])` to detect math refs.
+`TransformToCatalogAction` copies *all* original-ref attributes (including
+`filename`, `source`, domain columns) into the new `MathDataReference`, so the
+NaN guard silently falls through to a wrong key for every transform ref.
+
+The `name` column is always present in the DataFrame returned by
+`get_data_catalog()` because the base implementation calls
+`catalog.to_dataframe().reset_index()`, which promotes the catalog-key index into
+a regular column.
+
+### Mixed catalogs: NaN source for math references
+
+Rows produced by `to_dataframe().reset_index()` have `NaN` in the `source` column
+for math references, because derived references carry `source="transform"` or `""`.
 
 **`get_unique_short_names` must not receive NaN paths**
 
@@ -109,6 +166,55 @@ valid_sources = [s for s in df["source"].unique() if not pd.isna(s)]
 short_names = get_unique_short_names(valid_sources)
 ```
 
+### Time-range-aware readers — contract and efficiency
+
+`DataReference.getData(time_range=...)` passes `time_range` through `_load_data` into
+`reader.load(time_range=..., **attrs)` as a keyword argument alongside all other reference
+attributes.  Whether the reader uses it efficiently depends entirely on the backend API:
+
+| Backend API | Time-range behaviour |
+|-------------|----------------------|
+| `DSSFile.read_rts / read_its(path, start, end)` | **File-level windowing** — only the requested bytes are read |
+| HDF5 `get_data_for_catalog_entry(entry, time_window)` | **File-level windowing** |
+| `pyhecdss.get_ts(filename, path)` | No time-range parameter — full series loaded, slice in memory |
+| `pyhecdss.get_matching_ts(filename, path)` | No time-range parameter — full series loaded, slice in memory |
+| In-memory / callable readers | By definition full series — slice in memory |
+
+**Rule**: if the backend API supports native time windowing (e.g. `DSSFile.read_rts`), use
+it so that only the requested bytes are read from disk.  If the backend has no such API
+(e.g. high-level `pyhecdss.get_ts`), read the full series and slice by `time_range` before
+returning — do not skip the slice:
+
+```python
+def load(self, **attributes) -> pd.DataFrame:
+    time_range = attributes.get("time_range")
+    df = _load_full_series(attributes)      # only option with pyhecdss.get_ts
+    if time_range is not None:
+        start, end = pd.Timestamp(time_range[0]), pd.Timestamp(time_range[1])
+        df = df.loc[start:end]
+    return df
+```
+
+The `DataReference` cache is keyed by `(start, end)` so each unique window is stored
+independently and subsequent calls for the same window are served from memory.
+
+### `get_data_for_time_range` is a legacy hook — do not implement in new subclasses
+
+The `TimeSeriesDataUIManager.get_data(df)` method chooses its data-loading path based on
+whether `data_catalog` is set:
+
+```python
+if data_catalog is not None:
+    data = self.get_data_reference(r).getData(time_range=self.time_range)  # preferred
+else:
+    data, _, _ = self.get_data_for_time_range(r, self.time_range)          # legacy only
+```
+
+Any manager that sets `data_catalog` (via the `data_catalog` property) **never calls
+`get_data_for_time_range`**.  Do not override it in new subclasses — it is dead code
+once a catalog is wired up.  Remove any existing overrides that only duplicated the
+`getData(time_range=...)` logic.
+
 ## Subclass Migration Guide — primary_key / source_num Redesign
 
 This is a **breaking** redesign. The following concepts have been removed:

dvue/catalog.py

@@ -1070,6 +1070,45 @@ def remove(self, name: str) -> "DataCatalog":
         del self._references[name]
         return self
 
+    def rename(self, old_name: str, new_name: str) -> "DataCatalog":
+        """Rename a reference in the catalog (chainable).
+
+        Updates both the internal dictionary key and the reference's own
+        ``name`` attribute atomically.  Does not affect ``_source_index`` —
+        the source (file path) is unchanged by a rename.
+
+        Use this instead of mutating ``ref.name`` directly — direct mutation
+        leaves the dictionary key stale and causes ``catalog.get(ref.name)``
+        to raise ``KeyError``.
+
+        Parameters
+        ----------
+        old_name : str
+            Current name of the reference.
+        new_name : str
+            New name for the reference.
+
+        Raises
+        ------
+        KeyError
+            If no reference with ``old_name`` exists in the catalog.
+        ValueError
+            If ``new_name`` is already taken by a different reference.
+        """
+        if old_name not in self._references:
+            raise KeyError(f"No DataReference named {old_name!r} in catalog.")
+        if new_name != old_name and new_name in self._references:
+            raise ValueError(
+                f"Cannot rename {old_name!r} to {new_name!r}: a reference "
+                f"named {new_name!r} already exists in the catalog."
+            )
+        if old_name == new_name:
+            return self
+        ref = self._references.pop(old_name)
+        ref.name = new_name
+        self._references[new_name] = ref
+        return self
+
     def get(self, name: Optional[str] = None, **pk_kwargs: Any) -> DataReference:
         """Retrieve a :class:`DataReference` by name or by primary-key keyword arguments.
 

dvue/tsdataui.py

@@ -615,6 +615,13 @@ def get_table_column_width_map(self):
         cat = getattr(self, "data_catalog", None)
         if cat is not None and self._has_math_refs():
             df = cat.to_dataframe()
+            # Show the catalog key ('name') as a visible column so users can
+            # see what their transform/math refs are called.  Skip it when the
+            # subclass already provides a 'label' column as a cleaner display
+            # substitute (e.g. DSSDataUIManager injects label="" for raw refs
+            # and label=name for derived refs so the ugly raw DSS path is hidden).
+            if "name" not in column_width_map and "label" not in column_width_map:
+                column_width_map["name"] = "15%"
             for col in df.columns:
                 if col not in column_width_map and col not in ("geometry", "source"):
                     column_width_map[col] = "10%"

tests/test_catalog.py

@@ -1005,6 +1005,69 @@ def test_remove_nonexistent_raises(self):
         with pytest.raises(KeyError):
             cat.remove("nope")
 
+    # ------------------------------------------------------------------
+    # rename()
+    # ------------------------------------------------------------------
+
+    def test_rename_updates_key(self, simple_df):
+        cat = DataCatalog(primary_key=["name"])
+        cat.add(DataReference(source="", reader=InMemoryDataReferenceReader(simple_df), name="old"))
+        cat.rename("old", "new")
+        assert "new" in cat
+        assert "old" not in cat
+
+    def test_rename_updates_ref_name_attribute(self, simple_df):
+        cat = DataCatalog(primary_key=["name"])
+        cat.add(DataReference(source="", reader=InMemoryDataReferenceReader(simple_df), name="old"))
+        cat.rename("old", "new")
+        assert cat.get("new").name == "new"
+
+    def test_rename_is_chainable(self, simple_df):
+        cat = DataCatalog(primary_key=["name"])
+        cat.add(DataReference(source="", reader=InMemoryDataReferenceReader(simple_df), name="old"))
+        result = cat.rename("old", "new")
+        assert result is cat
+
+    def test_rename_nonexistent_raises_key_error(self):
+        cat = DataCatalog(primary_key=["name"])
+        with pytest.raises(KeyError, match="nope"):
+            cat.rename("nope", "something")
+
+    def test_rename_to_existing_name_raises_value_error(self, simple_df):
+        cat = DataCatalog(primary_key=["name"])
+        cat.add(DataReference(source="", reader=InMemoryDataReferenceReader(simple_df), name="a"))
+        cat.add(DataReference(source="", reader=InMemoryDataReferenceReader(simple_df.copy()), name="b"))
+        with pytest.raises(ValueError, match="already exists"):
+            cat.rename("a", "b")
+
+    def test_rename_to_same_name_is_noop(self, simple_df):
+        cat = DataCatalog(primary_key=["name"])
+        cat.add(DataReference(source="", reader=InMemoryDataReferenceReader(simple_df), name="a"))
+        cat.rename("a", "a")  # no error, no change
+        assert cat.get("a").name == "a"
+
+    def test_rename_preserves_source_index(self, simple_df):
+        cat = DataCatalog(primary_key=["name"])
+        cat.add(DataReference(source="f1.csv", reader=InMemoryDataReferenceReader(simple_df), name="a"))
+        cat.add(DataReference(source="f2.csv", reader=InMemoryDataReferenceReader(simple_df.copy()), name="b"))
+        cat.rename("a", "a_new")
+        df = cat.to_dataframe()
+        assert "source_num" in df.columns  # two distinct sources still tracked
+
+    def test_rename_data_still_loadable(self, simple_df):
+        cat = DataCatalog(primary_key=["name"])
+        cat.add(DataReference(source="", reader=InMemoryDataReferenceReader(simple_df), name="old"))
+        cat.rename("old", "new")
+        result = cat.get("new").getData()
+        assert isinstance(result, pd.DataFrame)
+
+    def test_rename_then_get_old_raises(self, simple_df):
+        cat = DataCatalog(primary_key=["name"])
+        cat.add(DataReference(source="", reader=InMemoryDataReferenceReader(simple_df), name="old"))
+        cat.rename("old", "new")
+        with pytest.raises(KeyError):
+            cat.get("old")
+
     def test_get_nonexistent_raises(self):
         cat = DataCatalog(primary_key=["name"])
         with pytest.raises(KeyError):