-
Notifications
You must be signed in to change notification settings - Fork 8
Fix stale CRS metadata after reprojection #237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| from ._registry import StatsPluginInterface, register | ||
| from odc.algo import enum_to_bool, erase_bad | ||
| from odc.algo import mask_cleanup | ||
| from odc.geo.xr import assign_crs | ||
| import logging | ||
|
|
||
| _log = logging.getLogger(__name__) | ||
|
|
@@ -123,6 +124,21 @@ def reduce(self, xx: xr.Dataset) -> xr.Dataset: | |
| gm = geomedian_with_mads(xx, **cfg) | ||
| gm = gm.rename(self._renames) | ||
|
|
||
| # geomedian_with_mads drops spatial_ref; re-attach from input | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question here, shouldn't
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, it should be fixed in geomedian_with_mads(). I can try to fix it in odc-algo if you'd prefer, or we can merge this workaround for now until the upstream is fixed. Once it is fixed there, I'll remove the workaround to keep the code clean.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| crs = getattr(xx.odc, "crs", None) # type: ignore[attr-defined] | ||
| if crs is not None: | ||
| gm = gm.copy() | ||
|
|
||
| # Remove stale CRS metadata | ||
| gm.attrs.pop("crs", None) | ||
| gm.attrs.pop("grid_mapping", None) | ||
| for v in gm.data_vars: | ||
| gm[v].attrs.pop("crs", None) | ||
| gm[v].attrs.pop("grid_mapping", None) | ||
|
|
||
| gm = gm.drop_vars(["spatial_ref", "crs"], errors="ignore") | ||
| gm = assign_crs(gm, crs=crs) | ||
|
|
||
| return gm | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,8 @@ install_requires = | |
| botocore | ||
| click>=8.0.0 | ||
| dask | ||
| datacube==1.9.5 | ||
| datacube>=1.9.5 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The datacube version is pinned because memory consumption balloons for odc-stats with later versions. I'm not involved in odc-stats so I don't know about the timeline for resolving that.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the context! I wasn't aware of the memory issue with later datacube versions. I made this change because my environment uses datacube 1.9.10, and the strict pin was causing dependency conflicts for me. Would it be possible to loosen the pin to allow users to manage their own datacube version if needed? |
||
| odc-stac==0.4.0 | ||
| distributed>=2025.4,<2025.9 | ||
| numpy | ||
| odc-cloud[ASYNC]>=0.2.5 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the output from
xr_reprojecthave the right CRS, so this issue should be fixed inside that function instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right - it should be fixed in xr_reproject().
I'm happy to attempt a fix in odc-geo if you'd prefer, otherwise we could merge this workaround for now and I'll open an issue upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without speaking for this repository, I think it would make sense to try to fix the problems in odc-algo and odc-geo. It sounds like you've found some bugs, and if I'm wrong and the methods are meant to behave the way they currently behave, you're more likely to get that feedback if you make PRs to those repositories.