[Review only] S1 RTC STAC-builder rebased (mirrors #180) - #214
Closed
lhoupert wants to merge 19 commits into
Closed
Conversation
Adds eopf_geozarr.stac.s1_rtc.build_s1_rtc_stac_item, which reads a consolidated S1 GRD RTC Zarr V3 store and returns a pystac.Item with SAR/SAT/projection extensions, WGS84 bbox derived via pyproj, and vv/vh asset sub-paths. Ascending orbit is preferred when both are present. Also adds the generate-stac-s1 CLI subcommand that prints the item as JSON. 8 unit tests cover all acceptance criteria from the plan. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Four functions in s1_ingest.py silently failed for s3:// paths:
- discover_s1tiling_acquisitions / discover_s1tiling_conditions used
pathlib.Path.glob(), which normalises s3:// to s3:/ and returns 0 matches
- ingest_s1tiling_acquisition / ingest_s1tiling_conditions coerced paths via
Path(), corrupting s3:// URIs, and called .exists() which always returns
False for S3 (existence checks now use path_exists() from fs_utils)
Adds three private helpers:
- _list_tifs(): uses s3fs.S3FileSystem.glob for s3:// prefixes
- _coerce_input_path(): preserves str for s3://, returns Path otherwise
- _rasterio_env(): rasterio.Env(AWSSession) context for s3:// paths;
strips https:// from AWS_ENDPOINT_URL since GDAL expects hostname only
Tested: rasterio.open("s3://...") confirmed working on OVH S3 with
AWS_S3_ENDPOINT or AWS_ENDPOINT_URL set; hostname-only required by GDAL.
Refs #139
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…ix_set Per Emmanuel's review on #173: - vv/vh asset hrefs now point to {store}/{orbit} (the zarr group that carries multiscales), matching how S2 reflectance assets point to measurements/reflectance. TiTiler reads tile_matrix_set from that group's multiscales attributes. - create_s1_store now writes tile_matrix_set into the orbit group's multiscales so TiTiler can discover the tiling scheme without error. - Updated test_asset_hrefs to assert the new orbit-group href. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…atibility TiTiler's _validate_zarr checks ds.rio.crs on the native resolution group (r10m). Without proj:code in the resolution group's attrs, rioxarray returns None and the group is excluded → groups=[] → bounds unpack error. S2 zarr already has proj:code at the resolution group level; mirror that for S1. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…176) The deployed titiler-eopf reader resolves a group's CRS via rioxarray, which reads a CF spatial_ref/crs_wkt coordinate -- not the GeoZarr proj:code attr. S1 GRD RTC stores carried only proj:code, so rio.crs was None and every multiscale group was rejected (HTTP 500 on render/info). The S2 converter already writes a spatial_ref grid-mapping; this brings S1 in line. Also stop writing tile_matrix_set into the orbit-group multiscales: the data-model owner confirmed it is not part of the S1 GRD RTC data model, and the data_api reader already treats it as optional (MISSING default). - add _add_grid_mapping() using pyproj CRS.to_cf() (the same source rioxarray uses, so no hard-coded projection) and call it for every resolution level (both store-creation paths) and the conditions group - remove _create_tile_matrix_set() and its multiscales entry - tests: assert no tile_matrix_set, spatial_ref present + rio.crs resolves to the native EPSG, conditions group carries grid_mapping This fixes at the source what data-pipeline currently works around in ingest_v1_s1_rtc.py (_patch_cf_grid_mapping / _patch_tile_matrix_limits). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
The previous S1 RTC preview rendered a single VH band as grayscale with an incorrect rescale (0,219), unsuitable for linear gamma0 RTC values. Declare a render-extension `renders.rgb` config producing the standard dual-pol false- colour composite (R=VV, G=VH, B=VV/VH ratio) with rescale 0–0.1 and tilesize 256, referencing the preferred orbit group. Downstream titiler-based services consume this to generate previews/tiles/tilejson. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A single [min, max] pair applies to all bands in titiler, so emitting three identical pairs was redundant (and forced collapse logic in the consumer). Same rendered output. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ender path (#246)
titiler-eopf reconstructs the store path as
s3://{bucket}/tests-output/{collection}/{item_id}.zarr and ignores the STAC asset
href, so the GeoZarr cube must be named after the item id (s1-rtc-{tile}) for new
tiles to preview. Parse the tile from the s1-rtc- prefix (was s1-grd-rtc-); the
item id (s1-rtc-{tile}) is unchanged, so filename == item id by construction.
Pairs with the data-pipeline direct-write change (store written at the tests-output
path) which replaces the #250 auto-copy. Dev-phase, no consumers — disposable test
data. Revert to s1-grd-rtc- when titiler-eopf#108 (resolve store from href) lands.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
build_s1_rtc_stac_item opened the cube with zarr.open_consolidated, which raises `ValueError: Consolidated metadata ... not found` when the store lacks root consolidated metadata. A per-tile cube grown by appending a time-slice to an existing same-orbit group ends up in exactly that state — re-consolidating an append on the S3 store is unreliable — so live STAC registration failed on the 2nd+ same-orbit acquisition for a tile (it broke the Pyrenees S1 RTC soak). The builder must not require consolidated metadata: titiler reads these stores fine without it. Fall back to a direct zarr.open_group(mode="r") read when the consolidated metadata is absent; re-raise any other ValueError. Adds a regression test: build from a 2-slice same-orbit store created with consolidate=False (fails before the fix in open_consolidated, passes after). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ring (#192) (#193) Per-acquisition previews render via titiler `sel=time={INTEGER index}` — a positional index that breaks once a cube's time axis goes non-monotonic (a cross-run append of an earlier-dated scene; proved on 31TEH: `[06-08, 06-07]` → the 06-07 item has no preview). Keeping positional indices correct would need cube reorder + full re-registration on every append (not scalable). The deployed titiler (v0.5.0) already does label-based `da.sel(..., method=...)` via `open_datatree(decode_times=True)`; the only gap is the cube's `time` array, which is a bare int64 with no CF datetime metadata, so it can't be a datetime index. Encoding it lets previews select by exact datetime, which works even on a non-monotonic axis — no reorder, register only the new item. titiler picks a multiscale level by zoom (previews use a coarse level), so `time` must resolve there. Putting it only at the orbit-group level and inheriting fails to open while r10m stays bare int64 (AlignmentError: int64 vs datetime64 on the shared dim). So encode `time` consistently at EVERY level: - new `_create_time_coordinate_array` + `TIME_CF_ATTRS` (units/calendar/ standard_name); created at every level in create + append-group paths. - the append write resizes/writes `time` at every level (was r10m only). - absolute_orbit/relative_orbit/platform stay at r10m (not selected on). - stored dtype stays int64 ns, so register_per_acquisition's raw read is unaffected. Adds TestTimeCFDatetime incl. exact datetime `.sel` on a non-monotonic axis at the native and a coarse level. 55 passed. Refs #192 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…brary (#195) (#196) * fix(s1-rtc): enrich cube STAC item + fix vv/vh asset model (#195) The S1 RTC cube STAC item carried duplicate vv/vh assets with byte-identical hrefs (bug #1), exposed no asset for the descending orbit group of a dual-orbit cube (bug #2), and was missing most of the metadata the S2 L2A reference carries. Rework `build_s1_rtc_stac_item`: - Asset model: replace the two indistinguishable vv/vh assets with one `gamma0-rtc-backscatter-{asc,desc}` asset per present orbit group, exposing VV/VH as named STAC 1.1 `bands` (no more duplicate hrefs), plus a `border-mask-{orbit}` asset for the valid-data mask. Each γ⁰ asset carries data_type/nodata/unit/gsd. - Identity: add `constellation`, `instruments: [c-sar]`, `gsd` (platform stays per-acquisition — a cube can mix S1A/S1C). - Projection: add proj:bbox, and proj:shape/proj:transform (best-effort from the r10m group attrs). - Datacube extension: cube:dimensions (time as a bounded extent — not a values list, the cube grows by appending — plus x/y) + cube:variables. - Descriptive: title/description; created (earliest acquisition, stable across rebuilds) / updated (build time) via the timestamps extension. - Fold the corrected render rescale (0.0,0.2) into `_rgb_render`. Out of scope (tracked follow-ups): statistics (#157), processing:software / DEM lineage, and per-product CDSE fields are not in the store; per-acquisition item construction is moved in a follow-up commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(s1-rtc): move per-acquisition + coverage STAC construction into the library Consolidate the *pure* S1 RTC STAC construction in eopf_geozarr.stac.s1_rtc so item shape has a single owner (the library); registration (STAC-API upsert, S3 alternates, gateway rewrite, TiTiler links, CDSE discovery) stays in data-pipeline. Adds, ported from the data-pipeline scripts (deployment URLs stripped): - acquisition_id(tile_id, when) - build_s1_rtc_per_acquisition_items(store, *, orbit, collection_id) -> list[pystac.Item]: one single-datetime item per cube time slice of an orbit, reoriented to that orbit (sat:orbit_state + render expression + only that orbit's γ⁰/mask assets), per-slice platform, datacube structure dropped (a single acquisition is not a cube). Deployment-agnostic: carries the render config + datetime; the registration layer derives the cube-endpoint TiTiler links (sel=time={datetime}) from it. - Slice / pick_slice / slice_coverages: coverage-based preview-slice selection (reads border_mask at r720m). Ported the construction unit tests (pick_slice, slice_coverages, and the per-acquisition construction subset); the link/thumbnail/alternate-asset and rescale-override tests stay in data-pipeline as registration concerns. Follow-up (separate data-pipeline PR): bump the data-model pin, delete the moved functions from register_per_acquisition.py / register_v1_s1_rtc.py, and rewire them to consume this library output. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(s1-rtc): per-acquisition items carry their own orbit footprint Review follow-up. A per-acquisition item cloned the cube base, so on a dual-orbit cube every item inherited the WGS84 union bbox/geometry of both orbits and the preferred (ascending) orbit's proj:bbox — advertising a footprint wider than, and a data bbox from the wrong orbit relative to, the acquisition's own orbit. Recompute bbox/geometry/proj:bbox from the run orbit's spatial:bbox (factored the bbox->polygon construction into `_bbox_to_geometry`, shared with the cube builder), and give per-acquisition items a single-acquisition description rather than the cube's "datacube" wording. Tests: per-acq footprint = run orbit (not the union); `created` = earliest acquisition (idempotent across rebuilds); ValueError on an invalid or absent orbit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(s1-rtc): omit orbit_state on dual-orbit cubes; expose datacube dimension sizes Two review observations from inspecting the live items: - sat:orbit_state is single-valued, so on a dual-orbit cube (both ascending and descending groups present) a single value mislabels half the slices. Set it (and declare the SAT extension) only for a single-orbit cube; per-acquisition items, which are single-orbit, still carry the real per-orbit value. Ensure per-acq items declare the SAT extension even when cloned from a dual-orbit base that omitted it. - cube:dimensions previously carried only `extent`, so the number of elements per dimension wasn't visible. The time axis is irregularly sampled, so list its discrete `values` (count = number of acquisitions); the regular x/y axes get a `step` (size derivable from extent; exact pixel count is also in proj:shape). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(s1-rtc): datacube time axis carries extent only (drop values enumeration) Enumerating every acquisition as cube:dimensions.time.values does not scale as the cube is appended over the mission. No STAC extension provides a scalable temporal element-count field (datacube temporal_dimension offers only values/step/extent, and step is null for S1's irregular sampling; projection proj:shape is spatial-only; raster has no dimension shape). So the time axis carries a bounded `extent` only. x/y element counts remain available via proj:shape (exact) and the datacube x/y extent+step. The number of acquisitions is obtained from the per-acquisition collection (STAC search count), which stays correct as the cube grows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(s1-rtc): correct datacube variable_type, platform value, created, time values Review of the live items surfaced several metadata issues: - cube:variables used `type` (a non-standard, silently-ignored key) instead of the datacube extension's `variable_type`; border_mask is now `auxiliary`, vv/vh `data`. - per-acquisition `platform` carried the store's short code (e.g. `s1a`); normalize to the STAC convention `sentinel-1a` (mirrors the Sentinel-2 reference). - `created` (timestamps ext) was set to an acquisition time, which misuses the field (it means the item's creation instant); omit it and keep `updated` = build time. - the cube's irregular time axis now lists its discrete `values` (the acquisition instants) so the number of time steps is visible; the regular x/y axes stay extent + step (their pixel count is in proj:shape). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(s1-rtc): robust EPSG parse for datacube reference_system + clarify per-acq proj inheritance Review follow-ups (no behaviour change for the EPSG:NNNN stores in use): - derive the datacube reference_system via pyproj.CRS(...).to_epsg() instead of splitting the proj:code string, so non-"EPSG:NNNN" CRS forms don't break it. - comment why per-acquisition items recompute proj:bbox from their orbit but inherit proj:code/shape/transform (the MGRS grid is shared across orbits). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(s1-rtc): note the dual-orbit time axis spans both orbits in the datacube A dual-orbit cube merges two per-orbit sub-cubes (disjoint time axes) onto a shared grid. Modelling orbit as a dimension would imply a sparse, mostly-empty orbit×time grid; instead orbit stays an attribute of each acquisition, conveyed via the per-orbit assets. Add a `description` on the time dimension so the merged axis isn't misread. Single-orbit cubes get no note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(s1-rtc): distinct per-acquisition titles; clearer asset titles - per-acquisition items inherited the cube's "… — tile {id}" title, so every scene in the acquisitions collection was titled identically. Give each a title with its datetime + orbit so siblings are distinguishable. - simplify the border-mask asset title ("Valid-data mask") and make the zarr-store title say "Sentinel-1" for naming consistency. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…v/vh pyramid (#197) * perf(s1-rtc): shard the conditions arrays like the vv/vh pyramid A real S1 RTC cube is 3807 objects / 3.5 GB, of which 3604 (94.7%) are the conditions/gamma_area_<relorbit> arrays: [10980,10980] float32, inner chunk 366², NO sharding_indexed -> ~900 tiny chunk objects each. They are time-invariant yet dominate the object count, which dominated the ingest's S3 upload wall-time (a live pod sat ~34 min in "Uploading store" at 9 millicores). The vv/vh/border_mask display pyramid is already sharded (one shard per time slice over the full spatial extent, inner 366²). Apply that same existing layout to the condition arrays: add shards=(h, w) to the one create_array in ingest_s1tiling_conditions. All condition arrays (gamma_area, lia, incidence_angle) share that write path and the same 2D full-resolution shape, so all collapse from ~900 chunk objects to 1 shard object (cube ~3807 -> ~210). calculate_aligned_chunk_size returns a divisor of the dimension, so (h, w) is a clean multiple of the inner chunk (Zarr v3 shard-divisibility). conditions arrays are NOT in TiTiler's render path (vv/vh/border_mask), so this does not touch the web-render layout; it only makes a client read a condition array in one ranged GET instead of ~900. Values are byte-identical. Tests: +2 (sharding codec present; 9 inner chunks -> 1 on-disk shard object + byte-identical roundtrip). 57 passed. Spec: claude-docs/specs/s1_gamma_area_sharding.md. Cross-repo Task T5 of data-pipeline/claude-docs/plans/s1_ingest_upload_perf.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LsWkVvRfkRzjqAMrzfmRP * docs(s1-rtc): record real-S3 sharding benchmark in the T5 spec Validated on the live OVH bucket (laptop->DE): object collapse 100->1 (prod ~900->1), PUT 1.7x faster even with batched concurrency on, divisibility valid at the production 10980² (aligned 366 divides 10980), reads byte-identical. Honest caveat recorded: a full-array sequential read is NOT faster sharded (same bytes, one un-parallelizable object) — the win is object-count (upload + listing) and windowed/partial cloud reads, not full-read throughput. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LsWkVvRfkRzjqAMrzfmRP * refactor(s1-rtc): trim the conditions-sharding comment to match vv/vh style Comment-only. The surrounding vv/vh sharding has no inline explainer; the long cloud-access rationale now lives in claude-docs/specs/s1_gamma_area_sharding.md. Keep only the non-obvious bits: why one shard, and the Zarr v3 shard-divisibility invariant. No behavior change (20 condition/shard tests green). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LsWkVvRfkRzjqAMrzfmRP * chore(s1-rtc): drop the claude-docs spec from this PR The data-model repo has no claude-docs/specs convention; the spec was noise for this PR's reviewers. The problem statement, real-S3 benchmark and migration note live in the data-pipeline plan + tracking issue EOPF-Explorer/data-pipeline#288 and PR #197's description. Also drop the now-dangling spec path from the code comment (the rationale stays inline). No behavior change (20 condition/shard tests green). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LsWkVvRfkRzjqAMrzfmRP --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
… writer) (#200) * perf(s1-rtc): shard the conditions arrays like the vv/vh pyramid A real S1 RTC cube is 3807 objects / 3.5 GB, of which 3604 (94.7%) are the conditions/gamma_area_<relorbit> arrays: [10980,10980] float32, inner chunk 366², NO sharding_indexed -> ~900 tiny chunk objects each. They are time-invariant yet dominate the object count, which dominated the ingest's S3 upload wall-time (a live pod sat ~34 min in "Uploading store" at 9 millicores). The vv/vh/border_mask display pyramid is already sharded (one shard per time slice over the full spatial extent, inner 366²). Apply that same existing layout to the condition arrays: add shards=(h, w) to the one create_array in ingest_s1tiling_conditions. All condition arrays (gamma_area, lia, incidence_angle) share that write path and the same 2D full-resolution shape, so all collapse from ~900 chunk objects to 1 shard object (cube ~3807 -> ~210). calculate_aligned_chunk_size returns a divisor of the dimension, so (h, w) is a clean multiple of the inner chunk (Zarr v3 shard-divisibility). conditions arrays are NOT in TiTiler's render path (vv/vh/border_mask), so this does not touch the web-render layout; it only makes a client read a condition array in one ranged GET instead of ~900. Values are byte-identical. Tests: +2 (sharding codec present; 9 inner chunks -> 1 on-disk shard object + byte-identical roundtrip). 57 passed. Spec: claude-docs/specs/s1_gamma_area_sharding.md. Cross-repo Task T5 of data-pipeline/claude-docs/plans/s1_ingest_upload_perf.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LsWkVvRfkRzjqAMrzfmRP * docs(s1-rtc): record real-S3 sharding benchmark in the T5 spec Validated on the live OVH bucket (laptop->DE): object collapse 100->1 (prod ~900->1), PUT 1.7x faster even with batched concurrency on, divisibility valid at the production 10980² (aligned 366 divides 10980), reads byte-identical. Honest caveat recorded: a full-array sequential read is NOT faster sharded (same bytes, one un-parallelizable object) — the win is object-count (upload + listing) and windowed/partial cloud reads, not full-read throughput. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LsWkVvRfkRzjqAMrzfmRP * refactor(s1-rtc): trim the conditions-sharding comment to match vv/vh style Comment-only. The surrounding vv/vh sharding has no inline explainer; the long cloud-access rationale now lives in claude-docs/specs/s1_gamma_area_sharding.md. Keep only the non-obvious bits: why one shard, and the Zarr v3 shard-divisibility invariant. No behavior change (20 condition/shard tests green). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LsWkVvRfkRzjqAMrzfmRP * chore(s1-rtc): drop the claude-docs spec from this PR The data-model repo has no claude-docs/specs convention; the spec was noise for this PR's reviewers. The problem statement, real-S3 benchmark and migration note live in the data-pipeline plan + tracking issue EOPF-Explorer/data-pipeline#288 and PR #197's description. Also drop the now-dangling spec path from the code comment (the rationale stays inline). No behavior change (20 condition/shard tests green). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LsWkVvRfkRzjqAMrzfmRP * fix(s1-rtc): heal a multiscale level missing `time` on append (robust writer) `ingest_s1tiling_acquisition` resized `level["time"]` on every multiscale level, assuming a fresh build created `time` at each level (#192). A cube built before #192 -- or left half-built by an interrupted append -- can carry `r10m/time` yet lack it at a coarser level, so the resize raised `KeyError: 'time'` and, because the append consistency check validated only CRS + shape, the ingest was non-convergent (observed on 30TWM). Before the per-level write loop, recreate any missing-level `time` from `r10m/time` (backfilling the existing slices so prior timestamps are preserved), or raise a clear error when the cube is inconsistent in a way a backfill cannot fix (a level's length disagrees with `r10m/time`, or `r10m` has slices but no `time`). This is the durable upstream counterpart to the data-pipeline guard (data-pipeline #294), making that orchestration-side mitigation belt-and-suspenders. Tests: 4 new cases (heal, no-op when healthy, raise on half-built, raise on missing r10m/time); full s1_ingest suite 61 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3qS75byrUuCSHFqcWi26B --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* fix(s1-rtc): write CF `_FillValue` on float arrays (data-model #172 parity) The S1 RTC ingest writer (`s1_ingest.py`) was never touched by #172, which added the CF `_FillValue` attribute to the S2 and S1-GRD paths so xarray can mask NaN nodata via `use_zarr_fill_value_as_mask=True` despite xarray #11345 — the zarr-level `fill_value` field alone is not surfaced through xarray's encoding. Consequently S1 RTC cubes carried only `grid_mapping` on their float bands, while S2 cubes carry `_FillValue` (`AAAAAAAA+H8=`) + standard_name + units, leaving S1 nodata unmaskable for generic xarray/CF consumers. Set the CF `_FillValue` (FillValueCoder-encoded, matching S2) plus standard_name and units on the float backscatter bands (vv/vh) at every multiscale level, and `_FillValue` on the float condition arrays (gamma_area, lia). The two near-identical vv/vh creation loops — new store vs. new orbit on an existing store — are unified into a shared `_create_band_arrays` helper so they can't drift again (the inline path previously lacked the metadata). Not ported (S2-specific or N/A to the RTC writer): the scale/offset + CastValue codec (S1 vv/vh are float32, unpacked), v0 deprecation (RTC is v1-only), and `sanitize_array_attrs` (s1_ingest writes controlled attrs zarr-direct and never copies source EOPF attrs). Tests: create-store unit, end-to-end ingest across both orbit-creation paths, and float-conditions coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sBdb17MWSAn7VEPSnqKJU * refactor(s1-rtc): address review — round-trip test, orbit-builder dedup, typed attrs - Add an end-to-end NaN-masking round-trip test (open with `use_zarr_fill_value_as_mask=True`, `to_masked_array()`), mirroring the S2 guarantee in `test_array_attrs.py`. Proves `_FillValue` actually masks nodata, not just that the attribute is present. - Unify the two orbit-creation paths — new store (`create_s1_store`) and new orbit added to an existing store (`ingest_s1tiling_acquisition`) — into a shared `_build_orbit_group` helper. Removes ~60 lines of duplication and fixes the inline path's missing `proj:code` on level groups (guarded by a new test). Net: `s1_ingest.py` shrinks while gaining the fix. - Model the backscatter band CF attrs as a `S1BackscatterAttrsJSON` TypedDict (PR review request), alongside the existing `Standard*CoordAttrsJSON` types. - Document the internal `xarray.backends.zarr.FillValueCoder` import. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sBdb17MWSAn7VEPSnqKJU --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…ent (#202) S1 RTC previews render the out-of-swath region as opaque black while the S2 reference renders it transparent. The cause is the *data*, not the metadata: #201 already gave vv/vh the same dtype/`fill_value`=NaN/ `_FillValue`/`grid_mapping` encoding as S2, but those are inert because no NaN is ever written — s1tiling stores `0.0` out of swath, and titiler treats `0` as valid data and paints it black. Mask nodata to NaN at the writer: - vv/vh: `np.where(border_mask == 0, NaN, ...)` — border_mask is the authoritative valid-data mask (0 = no-data). `_downsample_2d` already uses `np.nanmean` for floats, so NaN propagates to every overview level. - float conditions (gamma_area/lia): masked read off the GeoTIFF's declared nodata (border_mask is N/A for static conditions); a no-op when no nodata is declared. Only newly re-ingested cubes get NaN; existing cubes are remediated separately. Tests: NaN ⟺ border_mask==0 at native + overview levels, masking round-trips via `use_zarr_fill_value_as_mask`, conditions declared-nodata → NaN. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…d one (#203) S1 RTC cubes ended up with only one orbit consolidated on disk (staging: 30TWM asc✓/desc✗, 32TNN/30UVU asc✗/desc✓). Root cause: the pipeline ingests acquisitions one orbit at a time in separate pods, each of which strips all consolidated metadata (so `time` can resize), ingests its orbit, then calls `consolidate_s1_store(store, orbit_direction)` — which only re-consolidated the single orbit passed. So whichever orbit was ingested last is the only one left with on-disk consolidated metadata. Consolidate every orbit group present (iterate `root.groups()`), then the root. The minimal append-fetch already pulls all `zarr.json`, so both orbits' metadata is local — same reason the root consolidation already works. Signature unchanged (`orbit_direction` kept for logging/callers). Impact is cosmetic: the root consolidation is complete for both orbits and readers opening at the root get a synthesized child view (titiler renders the unconsolidated orbit fine). The fix matters for clients opening a single orbit group standalone (the STAC `<cube>.zarr/<orbit>` hrefs) with `consolidated=True`, which otherwise fall back to a listing. Test asserts each orbit group is consolidated *standalone* — checking via `root[orbit]` is a false-green because a consolidated root synthesizes the child's view. Existing cubes self-heal on their next re-ingest. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
The render expression emits 3 bands (vv; vh; vv/vh) but supplied a single rescale pair [0,0.2] applied to all three. The vv/vh ratio (natural range ~1-15) saturated under that stretch -> flat blue/purple wash, and low cross-pol water dropped to transparent, which made (correctly geolocated) ascending swaths look mislocated. Give each band its own pair: vv [0,0.4], vh [0,0.1], ratio [1,15]. Cosmetic preview-only change; pixel placement is unchanged. Verified live against the raster API and the S1 STAC tests. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
#205) Both the cube item and (by inheritance) its per-acquisition items now carry the STAC grid extension with grid:code = "MGRS-{tile}". This makes the tile a first-class queryable property: the acquisitions collection becomes natively tile-filterable in STAC Browser, and cube↔acquisition cross-links can filter on grid:code instead of an id-prefix LIKE. Cube-builder only — per-acquisition items inherit it via the existing build_s1_rtc_per_acquisition_items copy ({**base_dict} carries stac_extensions; the per-acq property denylist excludes grid:code). Claude-Session: https://claude.ai/code/session_019z3eVtkSNHhN9vHd8QqGcf Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
The rebase took main's uv.lock (--ours) at the stac-builder conflict; regenerate so the lockfile includes pystac (required by the S1 RTC STAC builder) on top of main's zarr-cm 0.4.1 / pyright dependency bump. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
The S1 RTC STAC-builder work (the same content as PR #180) rebased onto the rebased
s1-tiling. The diff here is the incremental RTC/STAC layer on top of the S1-tiling foundation.What to review
tests/test_s1_rtc_ingest.py: kept both the s3-URI discovery test and the masked-multiframe test (order differs harmlessly from the old branch; same two tests).uv.lockregenerated to includepystacon top of main's zarr-cm 0.4.1 / pyright bump (chore: regenerate uv.lock).Verification