Merge pull request #110 from fish-pace/copilot/clean-up-plan-show-variables

show_variables/open_dataset/open_mfdataset: always print `merge` key, remove per-group output blocks, use xarray instead of h5py

Author: eeholmes

src/point_collocation/core/plan.py

@@ -293,6 +293,7 @@ def open_dataset(
 
         if not silent:
             display_spec = {**spec, "open_kwargs": effective_kwargs}
+            display_spec.setdefault("merge", None)
             print(f"open_method: {display_spec!r}")
 
         if xarray_open == "datatree":
@@ -390,6 +391,7 @@ def open_mfdataset(
 
         if not silent:
             display_spec = {**spec, "open_kwargs": effective_kwargs}
+            display_spec.setdefault("merge", None)
             print(f"open_method: {display_spec!r}")
 
         result_list = results.results if isinstance(results, Plan) else list(results)
@@ -466,10 +468,8 @@ def show_variables(
             If the plan contains no granules.
         """
         from point_collocation.core._open_method import (
-            _GEOLOC_PAIRS,
             _apply_coords,
             _build_effective_open_kwargs,
-            _h5py_file_info,
             _merge_datatree_with_spec,
             _normalize_open_method,
             _open_and_merge_dataset_groups,
@@ -515,136 +515,15 @@ def show_variables(
                 spec = {**spec, "xarray_open": "dataset", "merge": None}
                 effective_kwargs = _build_effective_open_kwargs(spec.get("open_kwargs", {}))
 
-        # Print the spec summary.
+        # Print the spec summary.  Ensure "merge" is always present so the
+        # printed spec is unambiguous (it may be absent for the "auto" preset).
         display_spec = {**spec, "open_kwargs": effective_kwargs}
+        display_spec.setdefault("merge", None)
         print(f"open_method: {display_spec!r}")
 
-        def _seek_back() -> None:
-            if hasattr(file_obj, "seek"):
-                try:
-                    file_obj.seek(0)  # type: ignore[attr-defined]
-                except Exception:
-                    pass
-
-        # ---------------------------------------------------------------
-        # Fast path: use h5py for per-group metadata inspection.
-        # ---------------------------------------------------------------
-        h5_info = _h5py_file_info(file_obj)
-        _seek_back()  # ensure file_obj is at start for subsequent xarray opens
-        if h5_info is not None:
-            # Determine which groups to include based on merge spec.
-            merge = spec.get("merge")
-            if merge == "root":
-                relevant_groups: set[str] | None = {"/"}
-            elif isinstance(merge, list):
-                relevant_groups = set(merge)
-            else:
-                # "all" or no merge key: include all discovered groups
-                relevant_groups = None
-
-            all_var_names: set[str] = set()
-            merged_dims: dict[str, int] = {}
-            merged_vars: list[str] = []
-
-            for group_path, vars_dict in h5_info:
-                if relevant_groups is not None and group_path not in relevant_groups:
-                    continue
-
-                print(f"\nGroup {group_path}")
-                # Collect dimension sizes from variable shapes.
-                dim_sizes: dict[str, int] = {}
-                for vinfo in vars_dict.values():
-                    for dim, size in zip(vinfo["dims"], vinfo["shape"]):
-                        if dim and dim != "?":
-                            dim_sizes.setdefault(dim, size)
-                            merged_dims.setdefault(dim, size)
-                if dim_sizes:
-                    print(f"  Dimensions: {dim_sizes}")
-
-                if vars_dict:
-                    var_strs = [
-                        f"{vname}{vinfo['dims']}"
-                        for vname, vinfo in vars_dict.items()
-                    ]
-                    print(f"  Variables:  {', '.join(var_strs)}")
-
-                all_var_names.update(vars_dict.keys())
-                for vname in vars_dict:
-                    if vname not in merged_vars:
-                        merged_vars.append(vname)
-
-            # Flat merged summary across all relevant groups.
-            print(f"\nDimensions: {merged_dims}")
-            print(f"\nVariables: {merged_vars}")
-
-            # Geolocation detection — handles coords as dict, list, or 'auto'.
-            coords = spec.get("coords", "auto")
-            set_coords_val = spec.get("set_coords", True)
-            if isinstance(coords, dict):
-                lat_name = coords.get("lat")
-                lon_name = coords.get("lon")
-                if (
-                    lat_name and lon_name
-                    and lat_name in all_var_names
-                    and lon_name in all_var_names
-                ):
-                    print(f"\nGeolocation: ({lon_name!r}, {lat_name!r}) detected")
-                else:
-                    print(
-                        f"\nGeolocation: NONE detected with "
-                        f"'coords': {coords!r}, 'set_coords': {set_coords_val!r}."
-                    )
-            elif isinstance(coords, list):
-                missing = [n for n in coords if n not in all_var_names]
-                if not missing:
-                    geo_pairs = [
-                        (lon_n, lat_n)
-                        for lon_n, lat_n in _GEOLOC_PAIRS
-                        if lon_n in coords and lat_n in coords
-                    ]
-                    if geo_pairs:
-                        lon_n, lat_n = geo_pairs[0]
-                        print(f"\nGeolocation: ({lon_n!r}, {lat_n!r}) detected")
-                    else:
-                        print(f"\nGeolocation: {coords!r} found in dataset")
-                else:
-                    print(
-                        f"\nGeolocation: NONE — specified coords {missing!r} not found."
-                    )
-            else:
-                # 'auto': check _GEOLOC_PAIRS
-                geo_pairs_found = [
-                    (lon_n, lat_n)
-                    for lon_n, lat_n in _GEOLOC_PAIRS
-                    if lon_n in all_var_names and lat_n in all_var_names
-                ]
-                if geo_pairs_found:
-                    lon_n, lat_n = geo_pairs_found[0]
-                    print(f"\nGeolocation: ({lon_n!r}, {lat_n!r}) detected")
-                else:
-                    print(
-                        f"\nGeolocation: NONE detected with "
-                        f"'coords': {coords!r}, 'set_coords': {set_coords_val!r}. "
-                        "Try open_method='datatree-merge' or specify "
-                        "open_method={'coords': {'lat': '...', 'lon': '...'}}."
-                    )
-
-            return
-
         # ---------------------------------------------------------------
-        # Fallback: use xarray to inspect the file.
+        # Use xarray to inspect the file (same lazy path as open_dataset).
         # ---------------------------------------------------------------
-        # For "auto" mode, probe the first granule to resolve which open
-        # path to use (dataset vs. datatree).
-        if xarray_open == "auto":
-            try:
-                spec = _resolve_auto_spec(file_obj, spec)
-                xarray_open = spec["xarray_open"]
-                effective_kwargs = _build_effective_open_kwargs(spec.get("open_kwargs", {}))
-            except ValueError:
-                xarray_open = "dataset"
-                spec = {**spec, "xarray_open": "dataset"}
-
         dt = None
         merge = spec.get("merge")
         if xarray_open == "datatree":

tests/test_plan.py

@@ -3504,12 +3504,12 @@ def test_open_dataset_lazy_access_after_merge_does_not_raise(
 
 
 # ---------------------------------------------------------------------------
-# Tests for show_variables with h5py fast path (Task 3)
+# Tests for show_variables (xarray path)
 # ---------------------------------------------------------------------------
 
 
-class TestShowVariablesH5py:
-    """Tests for show_variables() using h5py fast path."""
+class TestShowVariables:
+    """Tests for show_variables() using the xarray path (no h5py)."""
 
     def _make_plan(self, tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch, nc_path: str) -> Plan:
         mock_ea = MagicMock()
@@ -3559,23 +3559,46 @@ def test_show_variables_flat_file_prints_dims_vars_geo(
         assert "lon" in captured.out
         assert "Dataset Detail" not in captured.out
 
-    def test_show_variables_grouped_file_prints_groups_and_sst(
+    def test_show_variables_open_method_includes_merge_key(
         self,
         tmp_path: pathlib.Path,
         monkeypatch: pytest.MonkeyPatch,
         capsys: pytest.CaptureFixture,
     ) -> None:
-        """show_variables() with grouped HDF5 prints group info and sst variable."""
+        """open_method line always includes 'merge' key, even when it is None."""
+        nc_path = str(tmp_path / "flat.nc")
+        _make_l3_dataset([-90.0, 0.0, 90.0], [-180.0, 0.0, 180.0]).to_netcdf(
+            nc_path, engine="netcdf4"
+        )
+        p = self._make_plan(tmp_path, monkeypatch, nc_path)
+        p.show_variables()
+        captured = capsys.readouterr()
+        first_line = captured.out.splitlines()[0]
+        assert first_line.startswith("open_method:")
+        assert "'merge'" in first_line
+
+    def test_show_variables_grouped_file_auto_mode_shows_root_group(
+        self,
+        tmp_path: pathlib.Path,
+        monkeypatch: pytest.MonkeyPatch,
+        capsys: pytest.CaptureFixture,
+    ) -> None:
+        """show_variables() with grouped HDF5 in auto mode shows root group (no Group blocks).
+
+        In auto mode xarray opens only the root group, so subgroup variables
+        such as 'sst' are not listed. Users should pass
+        open_method='datatree-merge' to inspect all groups.
+        """
         nc_path = str(tmp_path / "grouped.nc")
         _make_grouped_nc(nc_path)
         p = self._make_plan(tmp_path, monkeypatch, nc_path)
         p.show_variables()
         captured = capsys.readouterr()
-        assert "Group /" in captured.out
-        assert "sst" in captured.out
+        # Per-group blocks are not printed
+        assert "Group /" not in captured.out
+        # Root group geolocation and summary lines are present
         assert "Geolocation" in captured.out
         assert "lon" in captured.out
-        # The flat merged summary should also be present
         assert "Dimensions:" in captured.out
         assert "Variables:" in captured.out
         # Dataset Detail section should NOT appear