Skip to content

Commit 1cd2c93

Browse files
alxmrsclaude
andcommitted
Address review: fair cold-vs-cold (.compute), to_pandas headline, doc fixes
Fairness: case 05's reference used `.load()`, which caches data in place on the forecasts/truth objects the SQL table also reads from — so running the reference after the SQL query (as the harness does) could serve it a warm read. Switch to `.compute()`, which returns a fresh array and leaves the inputs lazy. Verified: reading a window repeatedly in one process stays flat, and neither side warms the other. The other cases either reopen their data (06) or recompute eagerly with `chunks=None` (02-04), so they were already cold; only 05 leaked. Case 05: render the headline RMSE-by-lead table with `to_pandas()` instead of a hand-rolled print loop; clarify that the `chunks=` arg is the Arrow batch size, not a filter (no data dropped). Case 06: the docstring claimed "full ARCO-ERA5" as the dataset; the query aggregates one day's window (the WHERE prunes the read). Make that precise. Case 07: drop the "graduate into the package" paragraph; clarify that there is one dataset (UTM x/y as SQL input, EE's pixelLonLat as the independent reference), not the same image opened twice in two CRS. Docs: correct the profiling methodology note (the caching trap is in-place `.load()`, not `open_zarr` auto-caching); point the "earns its keep" section forward to Results/Analysis/Conclusion; move "Running the suite" above Results; link James Bourbeau's profile; show the to_pandas headline repr. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AWvrZYAT2NbuETBqNAN3o9
1 parent dfd0039 commit 1cd2c93

4 files changed

Lines changed: 82 additions & 49 deletions

File tree

benchmarks/geospatial/05_forecast_skill.py

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -90,11 +90,15 @@ def _open(url: str) -> xr.Dataset:
9090
def _reference_rmse(forecasts: xr.Dataset, truth: xr.Dataset) -> xr.DataArray:
9191
"""xarray reference: per (model, lead), align truth at valid_time, take RMSE.
9292
93-
The 64×32 windows are tiny, so the reference loads them and reduces in
94-
memory; the SQL side above stays lazy.
93+
The 64×32 windows are tiny, so the reference reads them into memory and
94+
reduces there; the SQL side above stays lazy. We use ``.compute()`` rather
95+
than ``.load()`` deliberately: ``.load()`` caches the data *in place* on the
96+
shared ``forecasts``/``truth`` objects (which the SQL table also reads from),
97+
which would let a profiled reference serve a warm read — ``.compute()``
98+
returns a fresh array and leaves the inputs lazy, so each measurement is cold.
9599
"""
96-
f = forecasts[_VAR].load()
97-
e = truth[_VAR].load()
100+
f = forecasts[_VAR].compute()
101+
e = truth[_VAR].compute()
98102
leads = f.prediction_timedelta.values
99103
per_lead = []
100104
for lead in leads:
@@ -141,6 +145,9 @@ def main() -> None:
141145
)
142146

143147
ctx = xql.XarrayContext()
148+
# chunks here is the Arrow batch (partition) size the table streams in, not a
149+
# filter — no data is dropped. truth spans only the valid-time window, so
150+
# time:100 makes it a single partition; forecasts stream a few inits at a time.
144151
ctx.from_dataset("forecasts", forecasts, chunks={"time": 6})
145152
ctx.from_dataset("era5", truth, chunks={"time": 100})
146153

@@ -170,18 +177,17 @@ def main() -> None:
170177

171178
show_result(got)
172179

173-
# Headline table: error growth with forecast horizon, both models.
174-
lead_days = got["lead"].values / np.timedelta64(1, "D")
175-
pangu_rmse = got.sel(model="pangu")
176-
graphcast_rmse = got.sel(model="graphcast")
177-
print("\n 2m-temperature RMSE (K) vs lead time — lower is better:")
178-
print(f" {'lead (days)':>12} {'Pangu':>9} {'GraphCast':>11}")
179-
for i in range(0, len(lead_days), 4):
180-
print(
181-
f" {lead_days[i]:>12.2f} "
182-
f"{float(pangu_rmse.isel(lead=i)):>9.3f} "
183-
f"{float(graphcast_rmse.isel(lead=i)):>11.3f}"
184-
)
180+
# Headline: error growth with forecast horizon, both models. The gridded SQL
181+
# result round-trips to a pandas table directly — index is lead (in days),
182+
# one column per model.
183+
table = (
184+
got.assign_coords(lead=got["lead"].values / np.timedelta64(1, "D"))
185+
.to_pandas()
186+
.T
187+
)
188+
table.index.name = "lead (days)"
189+
print("\n 2m-temperature RMSE (K) vs lead — lower is better:\n")
190+
print(table.iloc[::4].round(3).to_string())
185191

186192

187193
if __name__ == "__main__":

benchmarks/geospatial/06_zonal_vector.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,10 @@
3131
prunes it to one day), the regions are a second SQL table, and the spatial
3232
relationship is an ordinary ``BETWEEN``.
3333
34-
Dataset: full ARCO-ERA5 + a handful of continental-scale bounding boxes
35-
(longitudes in ERA5's 0–360°E convention).
34+
Dataset: the full ARCO-ERA5 archive opened *lazily* — the table spans the whole
35+
record, but the query aggregates only one day's window (the ``WHERE`` prunes the
36+
read; it is not a scan of the full archive) — plus a handful of continental-scale
37+
bounding boxes (longitudes in ERA5's 0–360°E convention).
3638
"""
3739

3840
from __future__ import annotations

benchmarks/geospatial/07_reproject_udf.py

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,16 +29,15 @@
2929
SELECT x, y, reproject(x, y)['lon'] AS lon, reproject(x, y)['lat'] AS lat
3030
FROM grid
3131
32-
**The reference is Earth Engine itself.** We open a UTM grid through
33-
[Xee](https://github.com/google/Xee) carrying ``ee.Image.pixelLonLat()`` — Earth
34-
Engine's *own* geodesy engine computes the true lon/lat of every UTM pixel
35-
centre. So this case validates our PROJ-in-SQL transform against a fully
36-
**independent** reprojection implementation (EE), not against PROJ again. They
37-
agree to sub-metre precision.
38-
39-
The UDF (built on ``pyproj``, vectorized over each Arrow batch) mirrors the
40-
``cftime()`` UDF already shipped in xarray-sql (see ``xarray_sql/cftime.py``);
41-
it could graduate into the package as ``xql.register_reproject_udf``.
32+
**The reference is Earth Engine itself.** There is *one* dataset: a single UTM
33+
grid opened through [Xee](https://github.com/google/Xee) carrying
34+
``ee.Image.pixelLonLat()``. Each pixel arrives with two things — its UTM ``x``/
35+
``y`` (the grid coordinates, our SQL input) and Earth Engine's *own* per-pixel
36+
``longitude``/``latitude`` (data variables, the reference). So we are not
37+
opening the same image twice in two CRS; we feed the UTM coordinates to the PROJ
38+
UDF and check the lon/lat it returns against EE's independently-computed lon/lat
39+
for the *same* pixels. The reference is a different geodesy engine, not PROJ
40+
again, and they agree to sub-metre precision.
4241
4342
PROJ's context is not thread-safe and DataFusion evaluates projection
4443
expressions concurrently, so we return *both* coordinates from one

docs/geospatial.md

Lines changed: 47 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,8 @@ computes the same numbers — at ERA5's real 0.25° global resolution.
2828

2929
The operations here aren't a set we hand-picked to suit SQL. They're taken from
3030
[**Large Scale Geospatial Benchmarks**](https://github.com/coiled/benchmarks/discussions/1545)
31-
(coiled/benchmarks #1545), a discussion James Bourbeau opened in 2024 asking the
31+
(coiled/benchmarks #1545), a discussion [James Bourbeau](https://github.com/jrbourbeau)
32+
opened in 2024 asking the
3233
geospatial and climate community a pointed question: what are the *end-to-end
3334
workflows* the Xarray/Dask ecosystem needs to handle smoothly at the
3435
100-terabyte scale? The replies are a representative survey of what geoscience
@@ -161,11 +162,22 @@ for both models, matches an xarray reference, and reproduces the published resul
161162
that GraphCast edges out Pangu at every lead — the classic "error grows with
162163
horizon" curve (≈0.3 K at 6 h rising to ≈2.5 K at 9 days):
163164

165+
The result round-trips to a `pandas` table directly (`got.to_pandas()`), RMSE in
166+
kelvin by lead time:
167+
164168
```
165-
lead (days) Pangu GraphCast
166-
0.25 0.336 0.296
167-
5.25 1.469 1.228
168-
9.25 2.814 2.380
169+
model graphcast pangu
170+
lead (days)
171+
0.25 0.296 0.336
172+
1.25 0.464 0.554
173+
2.25 0.608 0.734
174+
3.25 0.780 0.936
175+
4.25 0.988 1.191
176+
5.25 1.228 1.469
177+
6.25 1.470 1.747
178+
7.25 1.763 2.096
179+
8.25 2.092 2.489
180+
9.25 2.380 2.814
169181
```
170182

171183
## 5. Raster × vector zonal statistics is a range `JOIN`
@@ -252,18 +264,43 @@ a weight matrix. `xarray-sql` sits downstream of all that as a query front-end:
252264
once the data is openable as an `xarray.Dataset`, these everyday operations are
253265
expressible — and accessible — as SQL.
254266

267+
That is the qualitative boundary; the rest of this page puts numbers to it. The
268+
**Results** below report what each operation costs in SQL versus the array
269+
reference, **Analysis** explains *why* the relational form is slower and where the
270+
time goes, and the **Conclusion** turns the whole thing into a when-to-use-which.
271+
272+
## Running the suite
273+
274+
```shell
275+
python benchmarks/geospatial/02_climatology.py # inside the repo
276+
uv run benchmarks/geospatial/02_climatology.py # standalone (PEP 723 deps)
277+
```
278+
279+
Each script prints its SQL, runs the array reference, and asserts the two agree.
280+
See [`benchmarks/geospatial/README.md`](../benchmarks/geospatial/README.md) for
281+
the full list and dataset notes.
282+
255283
## Results
256284

257285
Correctness is the headline, but every case is also profiled. The numbers below
258286
come from [`run_perf.sh`](../benchmarks/geospatial/run_perf.sh) on a Google Compute
259287
Engine `e2-standard-8` (8 vCPU, 32 GB) in `us-central1` — in-region with the
260288
ARCO-ERA5 and WeatherBench 2 buckets, so the cloud read is fast. Each case runs
261289
**once per fresh process**, with no warmup, repeated five times: the SQL operation
262-
*and* the xarray reference each pay a **cold** read on every measurement. (A warm
263-
in-process loop would flatter the array side — `xr.open_zarr(chunks=None)` caches
264-
each variable after its first read, so the reference would serve later repetitions
265-
from RAM while the SQL side re-reads the store. One process per repetition makes
266-
both sides pay the read every time.)
290+
*and* the xarray reference each pay a **cold** read on every measurement.
291+
292+
Fairness here took some care, because the obvious trap is caching. A reference
293+
that calls `.load()` caches its data *in place* on the very object the SQL table
294+
also reads from, so a later read — even just running the reference after the SQL
295+
query in the same process — could be served warm. We close that two ways. The one
296+
case that loads shared objects (05, forecast skill) uses `.compute()` instead,
297+
which returns a fresh array and leaves the inputs lazy, caching nothing; the other
298+
references either reopen their data or recompute their reduction eagerly on every
299+
read (`chunks=None` is NumPy, not Dask, so there is no graph to keep warm). And
300+
`run_perf.sh` runs each case in a fresh process per repetition, ruling out any
301+
carryover between reps. We verified the result directly: reading a window
302+
repeatedly in one process stays flat, and running either side after the other
303+
speeds up neither — the SQL query and the reference do not warm each other.
267304

268305
| Case | Step | median (s) | stdev (s) | min (s) | max (s) | peak (MB) |
269306
|---|---|--:|--:|--:|--:|--:|
@@ -376,14 +413,3 @@ scale. The point of this suite is not to crown a winner but to show that the lin
376413
between the two is exactly where the operation is dense versus where it is
377414
relational, and that for a surprising share of geoscience, the operation is
378415
relational.
379-
380-
## Running the suite
381-
382-
```shell
383-
python benchmarks/geospatial/02_climatology.py # inside the repo
384-
uv run benchmarks/geospatial/02_climatology.py # standalone (PEP 723 deps)
385-
```
386-
387-
Each script prints its SQL, runs the array reference, and asserts the two agree.
388-
See [`benchmarks/geospatial/README.md`](../benchmarks/geospatial/README.md) for
389-
the full list and dataset notes.

0 commit comments

Comments
 (0)