feat: Single-COG/item reads on native grid - #79
Conversation
hrodmn
left a comment
hrodmn
left a comment
There was a problem hiding this comment.
@Berhinj thanks for opening this! The open_cog/open_cog_async methods will be useful. Looking at your example notebook makes me think that an open_item method would also be useful where you provide an entire STAC item and a list of bands. We would need to check the extent/shape of each geotiff asset to make sure they are equal but it could be a nice complement to the open_cog option for cases where there are multiple bands.
- Introduced a new helper function `_write_synthetic_cog` to streamline the creation of synthetic COGs with customizable parameters such as size, native resolution, and band count. - Updated the `synthetic_cog` fixture to utilize the new helper function, maintaining the original properties while allowing for more flexibility. - Introduction open_item - Undo lock and pyproject changes
Berhinj
changed the title
Berhinj
changed the title
|
@hrodmn thanks for your interest, I've put on the work and done a clean implementation now, tell me what you think |
| reprojection. Source ``nodata`` is set as ``_FillValue`` and any | ||
| ``scale``/``offset`` as ``scale_factor``/``add_offset`` so rioxarray's | ||
| ``mask_and_scale`` decoding applies them. |
There was a problem hiding this comment.
Can check with @hrodmn but I think this project is using Markdown formatting, which means you only need one backtick. Did AI write these docs?
There was a problem hiding this comment.
Yes I rely a lot on on claude opus as everyone for routines, especially for docstring and tests, for code like this one I still wrote a good chunk myself and made sure to review all ai written code, it is true though that I'm spending as much effort on reviewing ai written docstring
There was a problem hiding this comment.
regarding the backtick, indeed in markdown it is single tick but in docstring it is two, tbh I copied the existing style from the other .py files you have which is already using double tick, example: https://github.com/Berhinj/lazycogs/blob/47ae2b6e0c43db3db7d899fc7e564e7746a86f8f/scripts/prepare_benchmark_data.py#L69-L70
There was a problem hiding this comment.
I'm not worried about the docstring formatting in the context of this PR. All of the existing docstrings were written by AI and most of them are too verbose anyways. I'll open an issue to track a cleanup.
| ) -> DataArray: | ||
| """Open one COG at native resolution as an ``(band, y, x)`` DataArray. | ||
|
|
||
| Async variant of :func:`open_cog` for use inside a running event loop. |
There was a problem hiding this comment.
Ditto we're not using Sphinx docstring formatting in this project
There was a problem hiding this comment.
correct, it is not compatible with mkdocs, I was copying the style from the existing functions of the repo - example: https://github.com/Berhinj/lazycogs/blob/47ae2b6e0c43db3db7d899fc7e564e7746a86f8f/src/lazycogs/_grid.py#L55
happy to correct it but the rest of the repo is full of it, could we keep it in a seperate pr?
|
@kylebarron your comments on docstring formatting are valid but they're everywhere in the repo, it is an existing issue, I believe they should be addressed in a different PR globally instead - which I did here #87 |
|
@Berhinj thanks for working on this feature! I was out of office the last two weeks but will take a look soon. |
hrodmn
left a comment
hrodmn
left a comment
There was a problem hiding this comment.
Thanks for your patience with my review @Berhinj.
These features will be really useful but there is one large structural change that I want to see before merging. Right now the open_item and open_cog methods completely bypass the logic that makes the lazycogs arrays lazy.
Could these entrypoints use the same BackendArray pattern as lazycogs.open instead of calling GeoTIFF.read() during open_*()?
GeoTIFF.open() is still appropriate at open time because we need header metadata for shape, dtype, CRS, transform, and open_item grid validation. I think we could also add some public helper functions to help users get those attributes. The resulting DataArray could wrap a small native-grid backend in indexing.LazilyIndexedArray, with GeoTIFF.read(window=...) deferred to getitem/async_getitem.
The native backend would be much simpler than MultiBandStacBackendArray: it would hold validated asset descriptors (path, store, header metadata), translate xarray y/x indexing into an async_geotiff.Window, and read only that window when materialized. For open_item, it would read only the selected single-band assets and stack them directly, avoiding xr.concat.
| """ | ||
| resolved_store, path = resolve(href, store=store, path_fn=path_from_href) | ||
| geotiff = await GeoTIFF.open(path, store=resolved_store) | ||
| raster = await geotiff.read() |
There was a problem hiding this comment.
calling geotiff.read() will eagerly load the entire file into memory. I would prefer to set this up to load the file lazily then do window reads at the last second when they load the data via .load() or .compute() etc.
There was a problem hiding this comment.
Very valid comments, I'll work on this
3 participants
Motivation
I believe people would benefit from loading a single scene untouched on original grid, reusing lazycogs' existing pieces but still getting an xarray out of it.
Summary
Adds
lazycogs.open_cogandlazycogs.open_itemto read a single COG or STAC item at its native grid (no reprojection), reusing existingobstore/async-geotiff/rustaccomponents.