From 29b628567473707953b2d42d9e38acf1666b27cb Mon Sep 17 00:00:00 2001 From: Rajeev Jain Date: Sun, 6 Sep 2026 07:25:06 -0500 Subject: [PATCH] Report which rule a spatial selection used to choose faces The three selections disagree about what "in" means and none of them said so: subset_polygon selects by face centre, cross_section by intersection, and subset_bbox keeps a face only when its whole spherical footprint fits inside the box. Measured on an 81-face mesh of 5-degree cells, a box of lon 5-15 / lat 5-15 holds six face centres and returns one face, and the surviving face spans latitude 7.5000-12.5115 because the great-circle edge bulges poleward of the nodes it joins. subset_coverage now carries selection_rule, and for subset_bbox also n_face_centers_in_bounds. Dropping boundary faces happens on every bounding-box call, so it is reported and not warned about. A box narrower than one face was a separate problem: it selects nothing while sitting on top of the mesh, and the refusal told the caller to move it onto the mesh, which is where it already was. Measured at lon 6-11 / lat 6-11, one centre inside and zero faces returned. That case now gets its own repair. --- CHANGELOG.md | 18 ++++ docs/tools.md | 24 ++++- src/uxarray_mcp/domain/subset_coverage.py | 64 +++++++++++ src/uxarray_mcp/preconditions.py | 37 +++++-- src/uxarray_mcp/tools/advanced.py | 25 ++++- tests/test_subset_coverage.py | 124 ++++++++++++++++++++++ 6 files changed, 276 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a0e0a2..9ac44fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,24 @@ uses Semantic Versioning for public releases. ## Unreleased ### Fixed +- Spatial selections now say which rule chose the faces. The three operations + do not agree: `subset_polygon` selects by face centre, `cross_section` by + intersection, and `subset_bbox` keeps a face only when its whole spherical + footprint fits inside the box. That last one is much stricter than the name + suggests — measured on an 81-face mesh of 5-degree cells, a box of lon 5–15 / + lat 5–15 holds six face centres and returns one face, and the surviving face + spans latitude 7.5000–12.5115 because the great-circle edge bulges poleward + of the nodes it joins. `subset_coverage` now carries `selection_rule`, and for + `subset_bbox` also `n_face_centers_in_bounds`, so the gap between what a + caller asked for and what the geometry allowed is visible. Dropping boundary + faces happens on every bounding-box call, so it is reported and not warned + about. +- A bounding box that lands on the mesh and still selects nothing was told to + move onto the mesh, which is where it already was. A box narrower than one + face returns nothing while sitting on top of the mesh — measured at lon 6–11 / + lat 6–11, one face centre inside and zero faces returned. That case now gets + its own repair: widen the box, or use `subset_polygon`, which selects by + centre. - Array summaries no longer report NaN for every statistic as soon as one value is missing. `summarize_array` called plain `min`/`max`/`mean`, which propagate NaN, so a field masked over half its faces returned `min`, `max` diff --git a/docs/tools.md b/docs/tools.md index 951de89..aef6680 100644 --- a/docs/tools.md +++ b/docs/tools.md @@ -230,13 +230,31 @@ empty box returns a `subset_grid` of `n_face: 0`, a `variable_summary` of `shape: [0]`, a persisted handle and a next-steps list suggesting the caller plot it, which is the full shape of an answer describing nothing. +The three do not select the same way, so the block also reports which rule ran. +`subset_polygon` keeps a face when its centre is inside the polygon +(`selection_rule: face_center_inside`). `cross_section` keeps a face the line +crosses (`face_intersects_line`). `subset_bbox` keeps a face only when the +face's whole spherical footprint fits inside the box +(`face_bounds_within`), which is stricter than the name suggests: on a mesh of +5-degree cells centred on multiples of 5, a box of lon 5–15 / lat 5–15 holds +six face centres and returns one face. The block reports +`n_face_centers_in_bounds` next to `n_face_retained` so that gap is visible. +The footprint is spherical rather than the rectangle through the nodes — the +surviving face above spans latitude 7.5000–12.5115, the extra 0.0115 being the +great-circle edge bulging poleward of the nodes it joins. On a mesh whose nodes +sit exactly on the requested bound, that bulge alone drops the face. Use +`subset_polygon` with a rectangle when centre-based selection is what you want. + The repair names the argument the caller controls — `lon_bounds`/`lat_bounds`, `polygon_lon_lat`, or `latitude`/`longitude` — and quotes the longitude and latitude the mesh actually spans, since "nothing selected" does not say where to put the box and the `-180..180` against `0..360` mix-up is the most likely -way to arrive here. `cross_section` is the one case UXarray already caught: it -raises rather than returning empty, and that error is turned into the same -refusal so the extent reaches the caller. Any other error still propagates. +way to arrive here. A box that lands on the mesh and still keeps nothing gets a +different repair: a box narrower than one face selects nothing while sitting on +top of the mesh, so it is told to widen, not to move. `cross_section` is the one +case UXarray already caught: it raises rather than returning empty, and that +error is turned into the same refusal so the extent reaches the caller. Any +other error still propagates. `temporal_mean` and `anomaly` both reduce along `time`, and both return a full-length array whatever went in. Both report a **`temporal_coverage`** block diff --git a/src/uxarray_mcp/domain/subset_coverage.py b/src/uxarray_mcp/domain/subset_coverage.py index 7331c26..4ba69ee 100644 --- a/src/uxarray_mcp/domain/subset_coverage.py +++ b/src/uxarray_mcp/domain/subset_coverage.py @@ -18,6 +18,29 @@ was wrong; the longitude and latitude the mesh actually spans tells them where to put it, and in particular whether they have the ``-180..180`` against ``0..360`` convention backwards. + +The three operations do not select the same way, which is why the rule is +reported rather than assumed. ``subset_polygon`` keeps a face when its centre +falls inside the polygon. ``cross_section`` keeps a face when the line crosses +it. ``subset_bbox`` keeps a face only when the face's whole spherical +footprint fits inside the box, which is much stricter than a caller reading +"bounding box" expects. Measured on an 81-face mesh of 5-degree cells centred +on multiples of 5, a box of lon 5..15 / lat 5..15 holds six face centres and +``bounding_box`` returns one. The footprint is spherical, not the node +rectangle: the surviving face spans latitude 7.5000..12.5115, the extra 0.0115 +being the great-circle edge bulging poleward of the nodes it joins. On a mesh +whose nodes sit exactly on the requested bound, that bulge alone is enough -- +a 5-degree quad mesh built on nodes at 5, 10 and 15 keeps two of the four faces +whose centres are inside, the other two reaching 15.0136 against a bound of 15. + +That is correct spherical geometry, not a defect, so it carries no warning +code -- dropping boundary faces is what a bounding box does on every call, and +a code that fires every time teaches callers to ignore it. The count of face +centres inside the box is reported instead, so the gap is visible to a caller +who cares. It matters in one case beyond bookkeeping: a box small enough that +no face fits entirely inside returns nothing while sitting squarely on the +mesh, and telling that caller to "move the box onto the mesh" would send them +away from the fix. """ from __future__ import annotations @@ -53,11 +76,39 @@ def mesh_extent(grid: Any) -> dict[str, float] | None: } +def count_face_centers_in_bounds( + grid: Any, + lon_bounds: list[float], + lat_bounds: list[float], +) -> int | None: + """How many face centres fall inside a longitude/latitude box. + + This is the number a caller expects ``subset_bbox`` to return, so it is + worth reporting next to the number it actually returns. ``None`` when the + grid exposes no usable face coordinates, for the same reason + :func:`mesh_extent` returns ``None``: an unmeasured count would be quoted + as fact. + """ + try: + lon = np.asarray(grid.face_lon, dtype=float) + lat = np.asarray(grid.face_lat, dtype=float) + lon_lo, lon_hi = float(lon_bounds[0]), float(lon_bounds[1]) + lat_lo, lat_hi = float(lat_bounds[0]), float(lat_bounds[1]) + except (AttributeError, IndexError, TypeError, ValueError): + return None + if lon.size == 0 or lat.size == 0 or lon.shape != lat.shape: + return None + inside = (lon >= lon_lo) & (lon <= lon_hi) & (lat >= lat_lo) & (lat <= lat_hi) + return int(inside.sum()) + + def compute_subset_coverage( n_face_source: int, n_face_retained: int, *, extent: dict[str, float] | None = None, + selection_rule: str | None = None, + n_face_centers_in_bounds: int | None = None, ) -> dict[str, Any]: """Report how much of the source mesh a selection kept. @@ -70,11 +121,24 @@ def compute_subset_coverage( extent The source mesh's face-centre bounding box, if it could be measured. Carried so the refusal can name where the mesh actually is. + selection_rule + Which test decided each face: ``"face_bounds_within"``, + ``"face_center_inside"`` or ``"face_intersects_line"``. Reported + because the three operations answer different questions and a caller + comparing their counts has no other way to know that. + n_face_centers_in_bounds + Face centres inside the requested box, for ``subset_bbox`` only. The + gap between this and ``n_face_retained`` is the boundary faces whose + spherical footprint did not fit. """ coverage: dict[str, Any] = { "n_face_source": int(n_face_source), "n_face_retained": int(n_face_retained), } + if selection_rule is not None: + coverage["selection_rule"] = selection_rule + if n_face_centers_in_bounds is not None: + coverage["n_face_centers_in_bounds"] = int(n_face_centers_in_bounds) if extent is not None: coverage["source_extent"] = extent return coverage diff --git a/src/uxarray_mcp/preconditions.py b/src/uxarray_mcp/preconditions.py index 8b904b4..2b83bed 100644 --- a/src/uxarray_mcp/preconditions.py +++ b/src/uxarray_mcp/preconditions.py @@ -516,23 +516,42 @@ def evaluate_subset_preconditions( summary of ``shape: [0]``, a persisted handle and a next-steps list -- the full shape of an answer, describing nothing. Keeping *fewer* faces is not a defect and is not checked: that is what a subset is for. + + A box that lands on the mesh and still keeps nothing gets a different + repair. ``subset_bbox`` keeps a face only when the whole face fits inside + the box, so a box narrower than one face selects nothing while sitting on + top of it -- measured at lon 6..11 / lat 6..11 on a mesh of 5-degree cells, + one face centre inside and zero faces returned. Telling that caller to move + the box would send them away from the fix. """ retained = coverage.get("n_face_retained", 0) source = coverage.get("n_face_source", 0) + centers_inside = coverage.get("n_face_centers_in_bounds") argument = _SUBSET_ARGUMENTS.get(operation, "the selection") - repair = f"Move {argument} onto the mesh." - extent = coverage.get("source_extent") - if extent: - repair += ( - " The mesh spans longitude {lon_min:g} to {lon_max:g} and latitude " - "{lat_min:g} to {lat_max:g}; check the longitude convention too " - "(-180..180 against 0..360)." - ).format(**extent) + detail = f"{operation}: {retained} of {source} faces selected." + if centers_inside is not None: + detail += f" {centers_inside} face centres lie inside the box." + if centers_inside: + repair = ( + f"Widen {argument}. The box is on the mesh -- {centers_inside} face " + "centres fall inside it -- but a face is kept only when the whole " + "face fits, and none of them do. Give the box at least one full " + "cell of room, or use subset_polygon, which selects by face centre." + ) + else: + repair = f"Move {argument} onto the mesh." + extent = coverage.get("source_extent") + if extent: + repair += ( + " The mesh spans longitude {lon_min:g} to {lon_max:g} and latitude " + "{lat_min:g} to {lat_max:g}; check the longitude convention too " + "(-180..180 against 0..360)." + ).format(**extent) return [ _check( "subset_retains_faces", retained > 0, - f"{operation}: {retained} of {source} faces selected.", + detail, repair, ) ] diff --git a/src/uxarray_mcp/tools/advanced.py b/src/uxarray_mcp/tools/advanced.py index 7b88ea5..d76cd19 100644 --- a/src/uxarray_mcp/tools/advanced.py +++ b/src/uxarray_mcp/tools/advanced.py @@ -19,7 +19,11 @@ compute_scattered_coverage, compute_target_coverage, ) -from uxarray_mcp.domain.subset_coverage import compute_subset_coverage, mesh_extent +from uxarray_mcp.domain.subset_coverage import ( + compute_subset_coverage, + count_face_centers_in_bounds, + mesh_extent, +) from uxarray_mcp.domain.temporal_coverage import compute_temporal_coverage from uxarray_mcp.next_steps import call, needed from uxarray_mcp.preconditions import normalize_units @@ -262,7 +266,13 @@ def subset_bbox( "original_grid": summarize_grid(grid), "subset_grid": summarize_grid(subset_grid), "subset_coverage": compute_subset_coverage( - grid.n_face, subset_grid.n_face, extent=mesh_extent(grid) + grid.n_face, + subset_grid.n_face, + extent=mesh_extent(grid), + selection_rule="face_bounds_within", + n_face_centers_in_bounds=count_face_centers_in_bounds( + grid, lon_bounds, lat_bounds + ), ), "variable_summary": variable_summary, "result_handle": result_handle, @@ -371,7 +381,10 @@ def subset_polygon( "selected_face_count": int(selected_indices.size), "selected_face_indices_preview": selected_indices[:25].tolist(), "subset_coverage": compute_subset_coverage( - grid.n_face, int(selected_indices.size), extent=mesh_extent(grid) + grid.n_face, + int(selected_indices.size), + extent=mesh_extent(grid), + selection_rule="face_center_inside", ), "variable_summary": variable_summary, "result_handle": result_handle, @@ -453,7 +466,10 @@ def extract_cross_section( "variable_summary": None, "result_handle": None, "subset_coverage": compute_subset_coverage( - grid.n_face, 0, extent=mesh_extent(grid) + grid.n_face, + 0, + extent=mesh_extent(grid), + selection_rule="face_intersects_line", ), } empty = attach_provenance( @@ -513,6 +529,7 @@ def extract_cross_section( grid.n_face, subset_grid.n_face if subset_grid is not None else 0, extent=mesh_extent(grid), + selection_rule="face_intersects_line", ), "variable_summary": variable_summary, "result_handle": result_handle, diff --git a/tests/test_subset_coverage.py b/tests/test_subset_coverage.py index 4ba126f..c3c9895 100644 --- a/tests/test_subset_coverage.py +++ b/tests/test_subset_coverage.py @@ -25,6 +25,7 @@ from uxarray_mcp.domain.subset_coverage import ( compute_subset_coverage, + count_face_centers_in_bounds, mesh_extent, subset_coverage_warning_codes, ) @@ -83,6 +84,41 @@ def test_an_extent_is_carried_when_it_was_measured(): assert coverage["source_extent"] == extent +def test_the_selection_rule_is_carried_when_it_was_given(): + coverage = compute_subset_coverage(81, 9, selection_rule="face_bounds_within") + assert coverage["selection_rule"] == "face_bounds_within" + + +def test_centres_inside_are_reported_separately_from_faces_kept(): + coverage = compute_subset_coverage(81, 2, n_face_centers_in_bounds=4) + assert coverage["n_face_centers_in_bounds"] == 4 + assert coverage["n_face_retained"] == 2 + + +def test_a_grid_without_coordinates_has_no_centre_count(): + class Bare: + pass + + assert count_face_centers_in_bounds(Bare(), [0.0, 10.0], [0.0, 10.0]) is None + + +def test_malformed_bounds_yield_no_centre_count(): + class Small: + face_lon = np.array([1.0]) + face_lat = np.array([1.0]) + + assert count_face_centers_in_bounds(Small(), [0.0], [0.0, 10.0]) is None + + +def test_centres_are_counted_inclusively_on_the_bound(): + class Row: + face_lon = np.array([2.5, 7.5, 12.5]) + face_lat = np.array([7.5, 7.5, 7.5]) + + assert count_face_centers_in_bounds(Row(), [7.5, 12.5], [0.0, 10.0]) == 2 + assert count_face_centers_in_bounds(Row(), [20.0, 30.0], [0.0, 10.0]) == 0 + + def test_a_grid_without_coordinates_has_no_extent(): class Bare: pass @@ -204,6 +240,94 @@ def test_an_on_mesh_selection_completes_and_reports_what_it_kept( assert result["scientific_status"]["warning_codes"] == [] +@pytest.mark.parametrize( + ("operation", "kwargs", "rule"), + [ + ( + "subset_bbox", + {"lon_bounds": [5.0, 15.0], "lat_bounds": [5.0, 15.0]}, + "face_bounds_within", + ), + ( + "subset_polygon", + {"polygon_lon_lat": [[5.0, 5.0], [25.0, 5.0], [25.0, 25.0]]}, + "face_center_inside", + ), + ("cross_section", {"latitude": 20.0}, "face_intersects_line"), + ], +) +def test_each_operation_names_the_rule_it_selected_by( + regional_files, operation, kwargs, rule +): + """The three operations answer different questions about the same face.""" + grid_file, data_file, _ = regional_files + result = _analyze(grid_file, data_file, operation=operation, **kwargs) + assert result["subset_coverage"]["selection_rule"] == rule + + +def test_a_bounding_box_keeps_fewer_faces_than_it_has_centres(regional_files): + """A box keeps a face only when the whole face fits inside it. + + Measured on the fixture mesh, whose faces are 5 degrees across and centred + on multiples of 5: lon 5..15 / lat 5..15 holds six face centres and returns + one face. Five of the six extend up to half a cell past the box. The one + that fits spans latitude 7.5000..12.5115 -- the 0.0115 is the great-circle + edge bulging poleward of its own nodes, which is what drops a face whose + nodes sit exactly on the bound. + """ + grid_file, data_file, _ = regional_files + result = _analyze( + grid_file, + data_file, + operation="subset_bbox", + lon_bounds=[5.0, 15.0], + lat_bounds=[5.0, 15.0], + ) + coverage = result["subset_coverage"] + assert coverage["n_face_centers_in_bounds"] == 6 + assert coverage["n_face_retained"] == 1 + # Dropping boundary faces is what a bounding box does on every call, so it + # is reported and not warned about. + assert result["scientific_status"]["warning_codes"] == [] + + +def test_a_box_narrower_than_one_face_is_told_to_widen_not_to_move(regional_files): + """The box is on the mesh, so "move it onto the mesh" would be wrong.""" + grid_file, data_file, _ = regional_files + result = _analyze( + grid_file, + data_file, + operation="subset_bbox", + lon_bounds=[6.0, 11.0], + lat_bounds=[6.0, 11.0], + ) + assert result["outcome"] == "input_required" + failed = result["refusal"]["failed_checks"] + assert [check["id"] for check in failed] == ["subset_retains_faces"] + assert "1 face centres lie inside the box" in failed[0]["detail"] + repair = failed[0]["repair"] + assert "Widen lon_bounds/lat_bounds" in repair + assert "subset_polygon" in repair + # The extent belongs to the other repair; quoting it here would suggest the + # box is somewhere it is not. + assert "0..360" not in repair + + +def test_a_narrow_box_still_returns_the_faces_on_an_override(regional_files): + grid_file, data_file, _ = regional_files + result = _analyze( + grid_file, + data_file, + operation="subset_bbox", + lon_bounds=[6.0, 11.0], + lat_bounds=[6.0, 11.0], + acknowledge=OVERRIDE_TOKEN, + ) + assert result["outcome"] == "complete" + assert result["subset_coverage"]["n_face_retained"] == 0 + assert result["preconditions"]["status"] == "overridden" + + def test_a_cross_section_that_misses_no_longer_raises(regional_files): """UXarray raises here; the front door turns it into the refusal shape.""" grid_file, data_file, _ = regional_files