engine: carry the coregister/auto_reproject target on the parameter value (drop like=)#3380
Conversation
brendancol
left a comment
There was a problem hiding this comment.
Review: assume coregister=True when like= is given
The change does what it says and the logic holds up across the kwarg combinations I worked through.
Blockers
None.
Suggestions
-
xrspatial/geotiff/tests/test_xarray_backend_like_default_coregister_3379.py: the new tests all use a DataArraylike. The default-coregister line runs before the DataArray-vs-Dataset dispatch, so a Dataset template hits the same code, but a one-lineto_dataset()case would lock in that a Datasetlike=alone also coregisters (the #3376 suite only covers Datasetlikewith explicitcoregister=True).
Nits
None worth holding the PR for.
What looks good
- The guard
if 'coregister' not in kwargs and not kwargs.get('auto_reproject')is the right shape. An explicitcoregister=wins, an explicitauto_reproject=Truekeeps the lighter reproject, andauto_reproject=Falsestill falls through to the coregister default._xarray_backend.py:127. - The no-
likeValueError guard is untouched, so passing a coregister kwarg without a target still raises the pointed error. - The behavior change (a bare
like=used to be a plain windowed read) is intentional and is called out in both the module docstring anddocs/source/reference/geotiff.rst, withcoregister=Falsedocumented as the opt-out. - Existing #3376 engine tests (auto_reproject parity, GPU/.vrt rejection, naming) still pass.
Checklist
- Logic correct across coregister / auto_reproject / neither
- Backend coverage inherited from the accessor (CPU-only coregister, GPU/.vrt rejected as before)
- No premature materialization (kwarg routing only)
- Behavior change documented
- Docstrings and reference docs updated
- Dataset
like=alone not directly tested (see suggestion)
brendancol
left a comment
There was a problem hiding this comment.
Follow-up review (after 9a58cdb)
The one suggestion from the first pass is addressed: test_dataset_like_alone_coregisters now covers a bare Dataset like= snapping onto its grid, alongside the DataArray cases. The follow-up is test-only, so no production code moved.
No new blockers, suggestions, or nits. Full file (7 cases) passes locally.
brendancol
left a comment
There was a problem hiding this comment.
Review: redesign — target rides on coregister=/auto_reproject= (drop like=)
Reviewed the reworked branch (41c3bce). The routing holds up across the kwarg combinations.
Blockers
None.
Suggestions
None.
Nits
_xarray_backend.py: if a caller passes a grid oncoregister=and a boolauto_reproject=True(not an array),coregisterwins silently andauto_reproject=Trueis forwarded but ignored by the accessor's precedence. Harmless and an odd thing to write, so not worth a guard — noting it for the record.
What looks good
- The array form is detected with
isinstance(x, (xr.DataArray, xr.Dataset))and rewritten to a plainTruebefore delegating, so the accessor'sif coregister:logic never sees an ambiguous array._xarray_backend.pyopen_dataset. The accessor file is untouched and its coregister suite (test_open_geotiff_coregister.py) still passes — confirmed locally. - The no-target guard uses
kwargs.get(k)truthiness, which is safe because that branch only runs when neither mode carries an array; barecoregister=True, loneresampling/var, the both-modes conflict, and the removedlike=all raise pointed errors (covered by tests). var=andresampling=ride through as modifiers of a Dataset/array target; Dataset dispatch +var=is tested.- Error messages name the replacement, so a day-one
like=user gets a clear migration path rather than an opaqueTypeError.
Checklist
- Truthiness contained at the engine boundary; accessor unchanged
- All error paths covered (removed like=, bare bool, lone modifier, both modes)
- coregister/auto_reproject parity with the accessor
- open_mfdataset shares the grid with coregister=
- docs (geotiff.rst + module docstring) rewritten to the new API
- accessor regression suite green
brendancol
left a comment
There was a problem hiding this comment.
Review: tight pass on the redesign (1993eb2)
One real bug, one nit, otherwise clean.
Blockers
-
_xarray_backend.pyopen_dataset, no-target branch: a falsycoregister/auto_reprojectpassed alone leaks into the standalone reader. The guard switched fromk in kwargstokwargs.get(k), sobackend_kwargs={"coregister": False}(alsoFalse/Noneonauto_reproject) is not flagged, stays inkwargs, and reachesopen_geotiff(filename_or_obj, **kwargs)— which has nocoregisterparameter. Result:TypeError: open_geotiff() got an unexpected keyword argument 'coregister', the opaque failure this engine is meant to turn into a pointed error.coregister=Falseshould mean "plain read", so strip the falsy mode flags before the standalone call rather than forwarding them:for k in _COREGISTER_ONLY_KWARGS: if not kwargs.get(k): kwargs.pop(k, None) da = open_geotiff(filename_or_obj, **kwargs)
Nits
- Same method: a bool
coregister=Truecombined with an arrayauto_reproject=<grid>(or vice versa) takes the array as the target, sets that mode, and forwards the leftoverTrueon the other flag; the accessor's coregister-precedence then silently wins. It's a contradictory input no one should write, so a guard is probably not worth it — flagging for the record.
What looks good
- Truthiness is contained at the engine boundary:
isinstancepicks the array target and the accessor only ever sees a plain bool. The accessor file is untouched and its suite (test_open_geotiff_coregister.py) still passes. - Error coverage is solid for the truthy cases: removed
like=, bareTruemodes, loneresampling/var, and a target on both modes all raise pointed errors with tests. var=/resampling=ride through as modifiers; Dataset dispatch +var=is tested;open_mfdatasetshares the grid.
Coverage gap (tied to the blocker)
- No test exercises a falsy mode flag alone. Add one asserting
backend_kwargs={"coregister": False}returns a plain read equal to the no-backend-kwargs read — it would have caught this.
Checklist
- Truthiness contained; accessor unchanged
- Falsy mode flag alone handled (blocker above)
- Truthy error paths covered
- Parity with the accessor for both modes
- Docs rewritten to the new API
brendancol
left a comment
There was a problem hiding this comment.
Follow-up: blocker fixed (d4ed9f7)
The falsy-mode-flag leak is closed. The no-target branch now pops the mode/modifier kwargs before the standalone read, so coregister=False / auto_reproject=False / coregister=None do a plain read instead of leaking an opaque TypeError. Verified False, auto_reproject=False, and None all return the same array as a no-backend-kwargs read.
Added test_falsy_mode_flag_alone_reads_plain (parametrized over both flags) asserting parity with the plain read. 53 engine + accessor tests pass, accessor suite unchanged.
The nit (contradictory bool-True + array on the other mode) is left as-is — a contrived input not worth a guard.
Closes #3379.
The
xrspatialengine took a coregistered read as two backend kwargs:like=<grid>for the target andcoregister=Truefor the mode. Since the target is the whole point of the read, this folds them into one: the target rides on the mode parameter's value.coregister=<DataArray|Dataset>reprojects and resamples onto that grid.auto_reproject=<DataArray|Dataset>reprojects onto its CRS but keeps the file's native resolution.resampling=/var=stay as modifiers of those reads.like=is removed. It shipped in v0.10.10 but is experimental (reader.coregister); passing it now raises aValueErrornaming the replacement.coregister=True/auto_reproject=True(no grid), a loneresampling/var, and a target on both modes each raise a pointedValueError.An xarray object is ambiguous in a boolean context, and the accessor's coregister logic is full of
if coregister:. So the engine detects the array form withisinstance, picks it as the target, and hands the accessor a plainTrue. The accessor is unchanged: thereselfis the target and the flags stay bools.Backend coverage: engine kwarg routing only, so it inherits the accessor's coverage. The coregister pipeline is CPU-only (numpy / dask+numpy); GPU and
.vrtare rejected as before.This supersedes the earlier approach on this branch (a bare
like=implyingcoregister=True).Test plan:
coregister=<grid>snaps onto the grid and matches the accessor'scoregister=Trueauto_reproject=<grid>reprojects at native resolution (shape != template) and matches the accessorvar=;open_mfdatasetwithcoregister=<grid>shares the gridlike=, bare bool modes, lone modifiers, target on both modestest_open_geotiff_coregister.py) still green, proving the accessor was not disturbed