ENH: skip redundant station coord reads for subsequent sweeps in DataTree by aladinor · Pull Request #337 · openradar/xradar

aladinor · 2026-02-27T10:42:49Z

Summary

Move station coordinates (latitude, longitude, altitude) to the root node as coordinates and drop them entirely from sweep nodes. This ensures the root is the single authoritative source for station location in a DataTree.

Rename boolean kwarg from site_coords to site_as_coords for clarity (per review)
Extract shared _apply_site_as_coords() helper into common.py, replacing duplicated if/else blocks across all backends
Refactor _assign_root() to return (root, cleaned_sweeps) — no hidden side effect
Drop station vars in _attach_sweep_groups() for backends using that path
Fix read-only array bug in _ipol_time using da.where instead of in-place mutation

Closes #334

Dependency

Do not merge until pydata/xarray#11230 is merged.

Xarray's DataTree currently does not inherit non-index coordinates from parent nodes. The xarray PR adds inherit="all" support, which will allow sweeps to access latitude, longitude, and altitude from the root via to_dataset(inherit="all").

Follow-up PR (after xarray PR merges)

Update the georeferencing module to use to_dataset(inherit="all") so ds.latitude, ds.longitude, ds.altitude are accessible on sweep datasets:

xradar/georeference/projection.py:129 — uses ds.latitude.values and ds.longitude.values
xradar/georeference/transforms.py:255 — uses ds.latitude.values and ds.altitude.values

Changes

xradar/io/backends/common.py: Add _apply_site_as_coords() helper; refactor _assign_root() to return cleaned sweeps; drop station vars in _attach_sweep_groups()
All backend open_dataset methods: Use _apply_site_as_coords(ds, site_as_coords)
All open_*_datatree() functions: Pass site_as_coords=False for sweep datasets
xradar/io/backends/cfradial1.py: Always assign station vars to sweeps before calling _apply_site_as_coords
xradar/io/backends/nexrad_level2.py, xradar/io/backends/uf.py: Use zip() to build sweep entries from cleaned ls_ds
xradar/util.py: Fix read-only array in _ipol_time with da.where
docs/history.md: Add changelog entry

Test plan

All 440 tests pass
TestStationCoordsOnRoot verifies root has station coords as coordinates and sweeps have none (ODIM, NEXRAD, GAMIC, CfRadial1)
Test data_vars counts updated to reflect 3 fewer vars per sweep
ruff check and black --check pass clean

…y_site_coords helper Pass site_coords=False for all sweeps when building a DataTree so that latitude/longitude/altitude are only placed as coordinates on the root node. Sweeps keep them as plain data variables, avoiding redundant I/O and coordinate promotion. Extracts the repeated if/else site_coords logic into a shared _apply_site_coords() helper in common.py, used by all 10 applicable backends. Closes openradar#334

CfRadial1's conform_cfradial2_sweep_group strips station variables (latitude, longitude, altitude) from sweep datasets. Previously these were only re-added when site_coords=True. With the DataTree optimization (site_coords=False), sweeps had no station vars at all, breaking georeference functions that access ds.longitude on sweep datasets. Now station vars are always assigned to sweeps, and _apply_site_coords controls whether they are promoted to coordinates or kept as data vars. Also adds a CfRadial1 test to TestStationCoordsOnRoot.

codecov · 2026-02-27T11:51:39Z

Codecov Report

❌ Patch coverage is 98.24561% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.51%. Comparing base (e59d9e6) to head (51a6fe7).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
xradar/io/backends/common.py	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #337      +/-   ##
==========================================
- Coverage   93.52%   93.51%   -0.02%     
==========================================
  Files          27       27              
  Lines        5717     5721       +4     
==========================================
+ Hits         5347     5350       +3     
- Misses        370      371       +1

Flag	Coverage Δ
notebooktests	`0.00% <0.00%> (ø)`
unittests	`93.51% <98.24%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…penradar#337)

@kmuehlbauer

Per reviewer feedback from @kmuehlbauer, rename the parameter across all backends, tests, and documentation for explicitness.

review-notebook-app · 2026-03-02T16:50:43Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

kmuehlbauer

There are occasions of site_coords replaced which are related to actual site_coords tuple, not the boolean kwarg.

Please revert these part.

The site_as_coords rename should only apply to the boolean kwarg, not to the site_coords property that returns the (lon, lat, alt) tuple.

NumPy/xarray may return non-writable arrays from .values and .loc, causing ValueError when _ipol_time tries to modify them in-place.

aladinor · 2026-03-13T02:52:54Z

Hi @kmuehlbauer, sorry about that. I overlooked the tuple property vs. the boolean kwarg distinction during the find & replace. I've reverted all the site_coords property/tuple references back to their original names. Only the boolean kwarg uses site_as_coords now.

Replace da.copy() + in-place mutation with da.where(da.notnull(), interpolated) to avoid full copy of the DataArray per review suggestion.

…vars - _assign_root returns (root, cleaned_sweeps) instead of mutating input list - _attach_sweep_groups drops station vars before attaching to DataTree - Use drop_vars(_STATION_VARS, errors="ignore") for idiomatic xarray

kmuehlbauer · 2026-03-23T09:41:47Z

@aladinor It would make sense, to get this in before #332. Could you spare some time to finish this up?

- Use to_dataset(inherit="all_coords") in georef tree functions so sweeps inherit lat/lon/alt from root - Pin xarray to git main (unreleased inherit support) - Bump requires-python to >=3.11 (matches xarray dev) - Add explicit setuptools package discovery

Sweep datasets no longer carry lat/lon/alt directly — they must inherit them from the root via to_dataset(inherit="all_coords").

Station coords now live on the root node only. Sweep datasets must use to_dataset(inherit="all_coords") to access lat/lon/alt.

Station coords are now scalar on root and inherited by sweeps. fix_sitecoords skips coords that are missing or already scalar.

kmuehlbauer · 2026-03-25T19:07:47Z

@aladinor You'd need to have the pin of xarray in environment.yml, too. It's used in RTD, without the docs build will always fail, or do I miss something?

Pin xarray to git main in environment.yml (RTD), ci/unittests.yml, and ci/notebooktests.yml so inherit='all_coords' is available everywhere.

aladinor · 2026-03-25T19:14:56Z

@kmuehlbauer you are right. I was trying to find the root of the failures. I've added the xarray git pin to environment.yml so RTD picks up inherit='all_coords'. Also pinned it in ci/unittests.yml and ci/notebooktests.yml via the pip section in the conda env files. These pins are temporary until xarray cuts a release

The @map_over_sweeps decorator previously used xr.map_over_datasets which passes DatasetView without inherited coordinates. Now it uses apply_to_sweeps which calls to_dataset(inherit="all_coords") so sweep datasets include lat/lon/alt from root. Also update Assign_GeoCoords notebook to fix per-ray station coords on root for mobile radar (DOW) before georeferencing.

kmuehlbauer · 2026-03-25T19:45:46Z

@aladinor Green lights. Great work! Shall I go ahead and merge?

aladinor · 2026-03-25T19:49:00Z

yeap. we are ready to go. Still dont have superpowers to merge :)

Pop `inherit` from kwargs before forwarding to func, defaulting to "all_coords" for backward compatibility. This lets callers control whether root coords (lat/lon/alt) are inherited onto sweep datasets. Using inherit=False prevents station coords from leaking onto sweeps after map_over_sweeps, preserving the root-only storage from PR openradar#337. Closes openradar#343

* FIX: allow passing inherit parameter to apply_to_sweeps Pop `inherit` from kwargs before forwarding to func, defaulting to "all_coords" for backward compatibility. This lets callers control whether root coords (lat/lon/alt) are inherited onto sweep datasets. Using inherit=False prevents station coords from leaking onto sweeps after map_over_sweeps, preserving the root-only storage from PR #337. Closes #343 * FIX: address PR review — document inherit values, use fixture - Document accepted inherit values with xarray cross-reference - Add cfradial1_sgp_dtree session fixture to conftest.py - Use fixture in new inherit tests instead of inline fetch+open * DOC: add PR #344 to changelog

aladinor added 2 commits February 27, 2026 06:40

aladinor force-pushed the feat/skip-redundant-station-coords-clean branch from 038f59a to 1eebc0e Compare February 27, 2026 11:45

CI: re-trigger CI after transient network failures

fbb8ab2

aladinor requested a review from kmuehlbauer March 2, 2026 00:22

aladinor added 2 commits March 1, 2026 19:24

CI: retry flaky Python 3.11 network failures

59b62e5

DOC: add changelog entry for skip redundant station coord reads (PR o…

363ef96

…penradar#337)

aladinor self-assigned this Mar 2, 2026

kmuehlbauer reviewed Mar 2, 2026

View reviewed changes

Comment thread xradar/io/backends/common.py Outdated

STY: rename site_coords to site_as_coords for clarity

fac3819

Per reviewer feedback from @kmuehlbauer, rename the parameter across all backends, tests, and documentation for explicitness.

aladinor requested a review from kmuehlbauer March 2, 2026 17:07

kmuehlbauer requested changes Mar 11, 2026

View reviewed changes

aladinor added 2 commits March 12, 2026 21:14

FIX: revert site_coords tuple property renames per review

7c692f8

The site_as_coords rename should only apply to the boolean kwarg, not to the site_coords property that returns the (lon, lat, alt) tuple.

FIX: copy read-only arrays before in-place mutation in _ipol_time

9fae98b

NumPy/xarray may return non-writable arrays from .values and .loc, causing ValueError when _ipol_time tries to modify them in-place.

aladinor requested a review from kmuehlbauer March 13, 2026 02:52

aladinor marked this pull request as draft March 13, 2026 04:10

aladinor marked this pull request as ready for review March 13, 2026 04:14

aladinor marked this pull request as draft March 13, 2026 04:36

kmuehlbauer reviewed Mar 13, 2026

View reviewed changes

Comment thread xradar/util.py Outdated

aladinor mentioned this pull request Mar 13, 2026

Add inherit='all_coords' option to DataTree.to_dataset() pydata/xarray#11230

Merged

3 tasks

aladinor added 7 commits March 13, 2026 18:35

FIX: use da.where instead of copy for read-only array in _ipol_time

f2d42c6

Replace da.copy() + in-place mutation with da.where(da.notnull(), interpolated) to avoid full copy of the DataArray per review suggestion.

FIX: use zip and unpack _assign_root tuple in nexrad datatree builder

e2abdd2

FIX: use zip and unpack _assign_root tuple in UF datatree builder

815cf4e

TST: assert station vars not in sweep data_vars or coords

9404e09

TST: update nexrad data_vars counts after station var removal

31c891e

TST: update UF data_vars counts after station var removal

82c6769

aladinor added 2 commits March 15, 2026 10:34

FIX: use cleaned sweeps from _assign_root in _get_required_root_dataset

97bb76f

STY: apply black formatting to nexrad and uf datatree builders

2f782a6

kmuehlbauer reviewed Mar 16, 2026

View reviewed changes

Comment thread xradar/util.py Outdated

kmuehlbauer and others added 2 commits March 16, 2026 10:22

Apply suggestion from @kmuehlbauer

3763606

lint

1c4e62e

kmuehlbauer mentioned this pull request Mar 23, 2026

ADD: Support reading NEXRAD Level 2 chunks from real-time S3 bucket #332

Merged

3 tasks

aladinor added 4 commits March 25, 2026 12:40

CI: install xarray dev to pick up inherit='all' from pydata/xarray#11230

f5ab0f3

STY: black format projection.py

bac31a7

ENH: use inherit='all_coords' in map_over_sweeps and export functions

c1c0b3b

Sweep datasets no longer carry lat/lon/alt directly — they must inherit them from the root via to_dataset(inherit="all_coords").

aladinor requested a review from kmuehlbauer March 25, 2026 18:35

aladinor marked this pull request as ready for review March 25, 2026 18:36

aladinor added 3 commits March 25, 2026 13:44

DOC: update notebooks to use to_dataset(inherit="all_coords")

d0ffc88

Station coords now live on the root node only. Sweep datasets must use to_dataset(inherit="all_coords") to access lat/lon/alt.

STY: black format HaloPhotonics notebook

d2e190e

DOC: update fix_sitecoords to handle scalar inherited coords

b3385e0

Station coords are now scalar on root and inherited by sweeps. fix_sitecoords skips coords that are missing or already scalar.

CI: pin xarray dev in all conda environment files

84212a3

Pin xarray to git main in environment.yml (RTD), ci/unittests.yml, and ci/notebooktests.yml so inherit='all_coords' is available everywhere.

kmuehlbauer approved these changes Mar 25, 2026

View reviewed changes

kmuehlbauer merged commit 5133922 into openradar:main Mar 25, 2026
11 checks passed

aladinor deleted the feat/skip-redundant-station-coords-clean branch March 25, 2026 21:25

aladinor mentioned this pull request Mar 27, 2026

ENH: allow passing inherit parameter to apply_to_sweeps / map_over_sweeps #343

Closed

aladinor mentioned this pull request Mar 27, 2026

FIX: allow passing inherit parameter to apply_to_sweeps #344

Merged

4 tasks

aladinor mentioned this pull request Apr 17, 2026

REL 0.12.0 #358

Closed

Conversation

aladinor commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Dependency

Follow-up PR (after xarray PR merges)

Changes

Test plan

Uh oh!

codecov Bot commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

review-notebook-app Bot commented Mar 2, 2026

Uh oh!

kmuehlbauer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aladinor commented Mar 13, 2026

Uh oh!

Uh oh!

Uh oh!

kmuehlbauer commented Mar 23, 2026

Uh oh!

kmuehlbauer commented Mar 25, 2026

Uh oh!

aladinor commented Mar 25, 2026

Uh oh!

kmuehlbauer commented Mar 25, 2026

Uh oh!

aladinor commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

aladinor commented Feb 27, 2026 •

edited

Loading

codecov Bot commented Feb 27, 2026 •

edited

Loading

kmuehlbauer left a comment •

edited

Loading

aladinor commented Mar 25, 2026 •

edited

Loading