odc-geo migration #1424

Ariana-B · 2023-03-24T01:33:44Z

Reason for this pull request

Geometry utilities have already been extracted from core into a separate package, odc-geo, however the code has not been replaced with calls to the odc-geo library.

Proposed changes

Replace all datacube.utils.geometry imports with the corresponding odc.geo imports
Remove geometry tests
Mark datacube geometry utilities as deprecated but still maintain backwards compatibility for now
Remove NetCDF Dataset wrapper
Mark xarray .geobox property as deprecated
Closes #xxxx
Tests added / passed
Fully documented, including docs/about/whats_new.rst for all changes

📚 Documentation preview 📚: https://datacube-core--1424.org.readthedocs.build/en/1424/

robbibt · 2023-03-26T22:25:45Z

Hey @Ariana-B, really looking forward to this - will be fantastic to have odc-geo functionality available natively in datacube. A question from a user perspective: one of the most useful things about odc-geo for my work is being able to access odc-geo functionality via the .odc extension directly from an xarray dataset, which at the moment is achieved by importing odc.geo.xr: https://odc-geo.readthedocs.io/en/latest/intro-xr.html

Will it be possible to access that .odc extension on data loaded from the datacube with the changes you're making here too? For example, I'd love to be able to do something like this (without having to manually import odc.geo.xr):

import datacube

dc = datacube.Datacube()

ds = dc.load(product=..., x=..., y=..., time=...)

# Access odc-geo Geobox for loaded data
ds.odc.geobox

…geometry tests

…Dataset wrapper, get most ingestion tests working

codecov · 2023-05-03T03:23:30Z

Codecov Report

Patch coverage: 97.01% and project coverage change: -6.34 ⚠️

Comparison is base (001870f) 91.97% compared to head (c905fd4) 85.63%.

Additional details and impacted files

@@               Coverage Diff               @@
##           develop-1.9    #1424      +/-   ##
===============================================
- Coverage        91.97%   85.63%   -6.34%     
===============================================
  Files              135      134       -1     
  Lines            15070    14958     -112     
===============================================
- Hits             13860    12809    -1051     
- Misses            1210     2149     +939

Impacted Files	Coverage Δ
datacube/drivers/netcdf/_write.py	`100.00% <ø> (ø)`
datacube/virtual/impl.py	`72.88% <66.66%> (-11.56%)`	⬇️
datacube/api/query.py	`93.75% <83.33%> (+0.03%)`	⬆️
datacube/api/core.py	`95.69% <100.00%> (+0.05%)`	⬆️
datacube/api/grid_workflow.py	`97.05% <100.00%> (ø)`
datacube/drivers/_types.py	`100.00% <100.00%> (ø)`
datacube/drivers/netcdf/writer.py	`94.70% <100.00%> (+0.06%)`	⬆️
datacube/drivers/postgis/_api.py	`87.26% <100.00%> (ø)`
datacube/drivers/postgis/_connections.py	`83.45% <100.00%> (ø)`
datacube/drivers/postgis/_spatial.py	`94.11% <100.00%> (+0.04%)`	⬆️
... and 19 more

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

datacube/api/core.py

SpacemanPaul · 2023-05-03T05:00:45Z

datacube/drivers/netcdf/_safestrings.py

Note to other reviewers: Ariana and I looked into it and the latest versions of the NetCDF library accept unicode strings, so this module is no longer required.

datacube/virtual/impl.py

docker/constraints.in

SpacemanPaul

Overall, looking great. See various minor inline comments, also you said "Mark datacube geometry utilities as deprecated but still maintain backwards compatibility for now" but that doesn't seem to have been done.

SpacemanPaul

Looks great, thanks Ariana.

As discussed, we'll give Kirill a chance to review before merging.

robbibt · 2023-05-04T02:25:37Z

Very excited about this PR! So at the moment (datacube < 1.9), the main way we access geo information about data loaded from the datacube is via the .geobox property that's automatically added to xarray data after you import datacube, e.g.:

ds.geobox

With odc.geo.xr imported, this geo information is automatically created via the new and more feature-rich .odc. extension instead, e.g.:

ds.odc.geobox

Is the plan that data loaded from datacube 1.9 will contain just the new automatically created .odc.geobox Geobox extension? Or will datacube 1.9 still add the older .geobox property to loaded data?

My feeling is that we don't need both - we should rely on the .odc.geobox one created by odc.geo instead as it's much more powerful, and it will potentially be confusing to users to have the same geobox acessible from different places (although it probably depends on how much backwards compatibility matters to us for the 1.9 release, as removing .geobox is likely to break user code).

Kirill888 · 2023-05-04T02:37:08Z

This document captures the main differences of odc-geo vs datacube.geometry.

via the .geobox property that's automatically added to xarray data after you import datacube

One could expose .geobox directly on an xarray for backwards compatibility using the following function in odc-geo

https://github.com/opendatacube/odc-geo/blob/b47103463f5beab27c48819744794e72c405bccc/odc/geo/_xr_interop.py#L762-L767

I think there is no issue having import odc.geo.xr present somewhere under import datacube code-path. It's not loaded by default in odc-geo imports because xarray is an optional and expensive dependency of odc-geo. This iss not the case with datacube.

Probably best to keep .geobox accessor for now, but it should generated deprecation warning on (first?) access. Attaching methods like this directly to xarray is now considered sub-optimal as there is now extension mechanisms present in xarray library.

Kirill888 · 2023-05-04T03:24:56Z

datacube/utils/geometry/__init__.py

@@ -44,7 +50,7 @@
    mid_longitude,
 )

-from .tools import (
+from .tools import (  # noqa


Adding these to __all__ below would remove the need for # noqa

Kirill888

Thank you @Ariana-B for going through this important but tedious change. I think this is good to merge. I would probably make sure that xx.geobox still works before making a release, it should issue deprecation warning stating that xx.odc.geobox is the new way to access this information.

Kirill888 · 2023-05-04T04:10:54Z

datacube/utils/xarray_geoextensions.py

 from datacube.utils.math import affine_from_axis
+from odc.geo import CRS, CRSError, wh_
+from odc.geo.geobox import GeoBox



One more thing that I have missed in the previous review. This file should just delegate geobox extraction to .odc.geo.xr with a warning, this extraction logic is in odc-geo.

I've implemented the change; let me know if it aligns with what you had in mind.

robbibt · 2023-05-04T04:31:21Z

Thanks @Kirill888, happy with us just marking the current .geobox as deprecated for now.

On a similar note, should these also be marked as deprecated? They all seem like things we can access more reliably via .odc.
https://github.com/opendatacube/datacube-core/pull/1424/files/2eb5ef6c81b46939126208c2a344ce74d9f08e44#diff-32d508f00b999e6cf6606a0676f5f5262674086e8cef094545e42eb776630555L182-L187

xarray.Dataset.geobox = property(_xarray_geobox)    # type: ignore
xarray.Dataset.affine = property(_xarray_affine)    # type: ignore
xarray.Dataset.extent = property(_xarray_extent)    # type: ignore
xarray.DataArray.geobox = property(_xarray_geobox)  # type: ignore
xarray.DataArray.affine = property(_xarray_affine)  # type: ignore
xarray.DataArray.extent = property(_xarray_extent)  # type: ignore

Ariana-B force-pushed the odc_geo_migration branch from 4247806 to c8e846f Compare March 24, 2023 05:06

robbibt mentioned this pull request Apr 20, 2023

Update functions to use odc-geo and add additional content to tests GeoscienceAustralia/dea-notebooks#1039

Merged

Ariana-B force-pushed the odc_geo_migration branch from 9d37018 to 4746b7f Compare May 1, 2023 05:01

Ariana Barzinpour added 16 commits May 3, 2023 03:06

replace datacube.utils.geometry imports with odc.geo imports; remove …

33f4110

…geometry tests

there was one more env variable I forgot to change

ee45add

didn't need to change that one

72c4107

fix geobox params

6c877d2

fix xr_coords error

c9bcc88

doctest and ReprojectInfo errors

38c33ea

fix can_paste in test

0ddbbf8

see if we can fix some tests

98a5e66

fix XY init error, attempt to fix Shape2D issue

fe6e90d

more fixes

9a5ab6f

more fixes

84abd76

more fixes

c860c35

fix is_affine_st nonetype error

a9fa55b

fix test_3d tests

38a8771

fix last Shape2D error, skip virtual test for now, stop using netcdf …

adbb2a7

…Dataset wrapper, get most ingestion tests working

fix test_load_data error, remove _safestrings.py

d21a56d

Ariana-B force-pushed the odc_geo_migration branch from dd302ef to d21a56d Compare May 3, 2023 03:09

Ariana-B changed the base branch from develop to develop-1.9 May 3, 2023 03:36

Ariana-B requested review from SpacemanPaul, omad and Kirill888 May 3, 2023 03:36

SpacemanPaul reviewed May 3, 2023

View reviewed changes

datacube/api/core.py Outdated Show resolved Hide resolved

SpacemanPaul reviewed May 3, 2023

View reviewed changes

remove check for tuple type

def1243

SpacemanPaul reviewed May 3, 2023

View reviewed changes

datacube/virtual/impl.py Outdated Show resolved Hide resolved

SpacemanPaul reviewed May 3, 2023

View reviewed changes

docker/constraints.in Show resolved Hide resolved

SpacemanPaul requested changes May 3, 2023

View reviewed changes

Ariana Barzinpour added 2 commits May 3, 2023 06:23

remove debug trace, regenerate constraints

6c88deb

add deprecation warning to geometry module

5ac2083

Ariana-B requested a review from SpacemanPaul May 3, 2023 06:36

remove deprecat from docker constraints

4b00978

SpacemanPaul approved these changes May 4, 2023

View reviewed changes

Ariana Barzinpour and others added 2 commits May 4, 2023 01:21

update whats_new

3ff17a1

Merge branch 'develop-1.9' into odc_geo_migration

2eb5ef6

Kirill888 reviewed May 4, 2023

View reviewed changes

Kirill888 approved these changes May 4, 2023

View reviewed changes

Kirill888 reviewed May 4, 2023

View reviewed changes

Ariana Barzinpour added 2 commits May 4, 2023 07:04

delegate geobox extraction to odc-geo and mark .geobox as deprecated

e3116d9

resolve remaining tests

c905fd4

Ariana-B merged commit 5c96e60 into develop-1.9 May 5, 2023

Ariana-B deleted the odc_geo_migration branch May 5, 2023 06:03

snowman2 mentioned this pull request May 6, 2023

CRS: CF grid_mapping attribute to encoding #1084

Closed

robbibt mentioned this pull request Jun 15, 2023

Refactor dea_tools.classification functions to use odc.geo GeoscienceAustralia/dea-notebooks#1076

Open

Ariana-B self-assigned this Jun 26, 2023

This was referenced Feb 6, 2024

Replace datacube GeoBox with odc-geo GeoBox? #1544

Closed

Use odc-geo GridSpec model internally #1545

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

odc-geo migration #1424

odc-geo migration #1424

Ariana-B commented Mar 24, 2023 •

edited

Loading

robbibt commented Mar 26, 2023 •

edited

Loading

codecov bot commented May 3, 2023 •

edited

Loading

SpacemanPaul May 3, 2023

SpacemanPaul left a comment

SpacemanPaul left a comment •

edited

Loading

robbibt commented May 4, 2023 •

edited

Loading

Kirill888 commented May 4, 2023

Kirill888 May 4, 2023

Kirill888 left a comment

Kirill888 May 4, 2023

Ariana-B May 5, 2023

robbibt commented May 4, 2023

odc-geo migration #1424

odc-geo migration #1424

Conversation

Ariana-B commented Mar 24, 2023 • edited Loading

Reason for this pull request

Proposed changes

robbibt commented Mar 26, 2023 • edited Loading

codecov bot commented May 3, 2023 • edited Loading

Codecov Report

SpacemanPaul May 3, 2023

Choose a reason for hiding this comment

SpacemanPaul left a comment

Choose a reason for hiding this comment

SpacemanPaul left a comment • edited Loading

Choose a reason for hiding this comment

robbibt commented May 4, 2023 • edited Loading

Kirill888 commented May 4, 2023

Kirill888 May 4, 2023

Choose a reason for hiding this comment

Kirill888 left a comment

Choose a reason for hiding this comment

Kirill888 May 4, 2023

Choose a reason for hiding this comment

Ariana-B May 5, 2023

Choose a reason for hiding this comment

robbibt commented May 4, 2023

Ariana-B commented Mar 24, 2023 •

edited

Loading

robbibt commented Mar 26, 2023 •

edited

Loading

codecov bot commented May 3, 2023 •

edited

Loading

SpacemanPaul left a comment •

edited

Loading

robbibt commented May 4, 2023 •

edited

Loading