feat: support bbox in in geometry #138

ljstrnadiii · 2024-02-13T04:06:00Z

This pr adds support for matching an existing xarray dataset that adheres to rioxarray standards and to support float64 precision with coordinates.

Motivating Use Case of `match_xarray`:

It will generally make it easier to merge datasets that come from somewhere else!
e.g. I have a ton of LiDAR transects as their own tifs. Now I can open them in rioxarray and pass them to load data with the correct transform once instead of resampling again to match.

Motivating Use Case of `use_coords_double_precision`:

more precise transforms if matching closely is important to people
allows a hacky and not exactly perfect approach to match to an existing xarray's transform if their dims are in float64
when using double precision geometry objects to match the geometry of an existing dataset, .getInfo() seems to change the values so slightly! This motivated np.testing.assert_all_close instead of np.testing.assert_equal

🦩

ljstrnadiii · 2024-02-13T04:19:14Z

Quick screen shot of how coords are changing before calling .getInfo(), after, and the inputs that come from raster.io.bounds(). I might be missing something here, but they are off just slightly. At least enough to for xr.merge to notice. Actually, I am seeing that if we call .bounds() on a ee.Geometry.Rectangle, we see this discrepancy. I would love to use np.testing.assert_equal instead of the close test if we can think of a way to prevent this or maybe this is an issue worth filing with ee folks.

ljstrnadiii · 2024-02-13T19:04:11Z

@dabhicusp @naschmitz @alxmrs (+others; not sure who to cc) would you all be receptive to something like this? Any thoughts?

ljstrnadiii · 2024-02-14T01:16:27Z

xee/ext_integration_test.py

@@ -41,17 +41,14 @@
 ]


-def _read_identity_pool_creds() -> identity_pool.Credentials:


Happy to revert to this, but it was easier for me to get tests up and running with google.auth.default, which looks for GOOGLE_APPLICATION_CREDENTIALS by default anyways.

ljstrnadiii · 2024-02-14T01:18:02Z

xee/ext_integration_test.py

+    # This is off slightly due to bounds determined by geometry e.g. .getInfo()
+    # seems to cause a super slight shift in the bounds. Thhe coords change before
+    # and after the call to .getInfo()!
+    np.testing.assert_almost_equal(


I really want this to pass with assert_equal.

This reverts commit 7efe992.

ljstrnadiii · 2024-02-14T19:06:25Z

I removed changes around ee credentials/auth in case that was the issue with the previous workflow run failure.

ljstrnadiii · 2024-02-14T20:31:22Z

I am not sure what's up with failing ci workflows. Looks like it may be a secret issue.

jdbcode · 2024-02-14T20:35:59Z

Thanks for the PR, Leonard! You can mostly ignore the ci workflows, they'll be run in a different environment. Note that currently we're on a biweekly issue and PR triage cycle. Thanks for your patience as we get this assigned for review!

alxmrs · 2024-02-15T15:45:28Z

xee/ext.py

@@ -40,6 +40,7 @@
 from xarray.backends import store as backends_store
 from xarray.core import indexing
 from xarray.core import utils
+import xarray as xr


Xarray is already imported.

alxmrs

Use double precision sounds like a useful feature, but I am uncertain about match_xarray. This doesn’t look like a standard part of rasterio — am I mistaken?
I would advise not modifying the base interface to achieve this kind of compatibility (given a review on mobile).

ljstrnadiii · 2024-02-15T20:08:24Z

Use double precision sounds like a useful feature, but I am uncertain about match_xarray. This doesn’t look like a standard part of rasterio — am I mistaken? I would advise not modifying the base interface to achieve this kind of compatibility (given a review on mobile).

I got some inspiration from rioxarray.reproject_match here, which is commonly used to merge datasets.
I am cool with dropping this in favor of double precision support if we can get this test to pass with assert equal not assert close somehow.
I would like a way to specify the exact transform of the Xee dataset without the issues I tried to highlight above--namely that bounds change ever so slightly with the ee.geometry interface before and after getInfo() call. Maybe we can support a tuple[float, float, float, float] type in the geometry as a compromise?
We could also drop the double precision arg and enable behind the scenes if the geometry or scale is float64 somehow?

alxmrs · 2024-02-16T03:42:04Z

xee/ext.py

@@ -1020,6 +1044,13 @@ def open_dataset(
        frameworks.
      ee_init_kwargs: keywords to pass to Earth Engine Initialize when
        attempting to auto init for remote workers.
+      use_coords_double_precision: Whether to use double precision for coordinates


Should float64 just be the default? I think float32 was an offhand desire to save bytes, but it could be simply incorrect.

Someone on the EE team will better be able to clarify.

Yeah, I think this might be a good idea. If people want float32, they can cast before they write things out.

alxmrs · 2024-02-16T03:47:41Z

namely that bounds change ever so slightly with the ee.geometry interface before and after getInfo() call. Maybe we can support a tuple[float, float, float, float] type in the geometry as a compromise?

I think this may exist already, or is close to existing. You don’t have to pass in ee.Geometry to set the geometry. Does proj’s CRS object provide one the way?

We could also drop the double precision arg and enable behind the scenes if the geometry or scale is float64 somehow?

I like this idea! It makes sense that it would be related to the projection.

ljstrnadiii · 2024-02-16T16:30:24Z

I think this may exist already, or is close to existing. You don’t have to pass in ee.Geometry to set the geometry. Does proj’s CRS object provide one the way?

Sorry, what may exist already or close to existing? Support for tuple[float, float, float, float] type in geometry? As far as I understand, you can pass geometry or None. If None, the bounds are taken from the CRS's area of use. This object doesn't have a set method for area_of_use as far as I can tell, which makes sense 🤷 .

I like this idea! It makes sense that it would be related to the projection.

Me, too, but I am not so sure there is a straightforward way to determine if the bounds provided are float32 or float64.

I am starting to think that the safest/simplest thing would be to change to float64 for coords to avoid truncation, and add support for a tuple[float, float, float, float] type in geometry. That would let me precisely define the dataset's transform. Thoughts? I can implement and push up those changes to get a sense of what that would look like if there are no major objections.

alxmrs · 2024-02-17T03:24:50Z

Sorry Leonard, I have to admit I’m not looking that closely. I see now that you’re right: the geometry arg should take a tuple of bounds. No objections to your proposal from my end! Thanks for your contribution. :)

…

On Fri, Feb 16, 2024 at 11:30 PM Leonard ***@***.***> wrote: I think this may exist already, or is close to existing. You don’t have to pass in ee.Geometry to set the geometry. Does proj’s CRS object provide one the way? Sorry, what may exist already or close to existing? Support for tuple[float, float, float, float] type in geometry? As far as I understand, you can pass geometry or None. If None, the bounds are taken from the CRS's area of use <https://pyproj4.github.io/pyproj/stable/api/crs/crs.html#pyproj.crs.CRS.area_of_use>. This object doesn't have a set method for area_of_use as far as I can tell, which makes sense 🤷 . I like this idea! It makes sense that it would be related to the projection. Me, too, but I am not so sure there is a straightforward way to determine if the bounds provided are float32 or float64. I am starting to think that the safest/simplest thing would be to change to float64 for coords to avoid truncation, and add support for a tuple[float, float, float, float] type in geometry. That would let me precisely define the dataset's transform. Thoughts? I can implement and push up those changes if there are no major objections. — Reply to this email directly, view it on GitHub <#138 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARXAB2V7NTXJ2ASLRZW6I3YT6CSZAVCNFSM6AAAAABDF2AQDOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBYHAZDAMJWGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

ljstrnadiii · 2024-02-17T16:03:49Z

@alxmrs I am not sure how you will feel about keeping geometry only or if you would prefer a bounds or bbox argument. I added the proposed changes. Also, apologies for the messy commit history, but the last three should be helpful in demonstrating things.

alxmrs · 2024-02-18T03:12:56Z

xee/ext.py

@@ -139,7 +139,7 @@ def open(
      crs: Optional[str] = None,
      scale: Optional[float] = None,
      projection: Optional[ee.Projection] = None,
-      geometry: Optional[ee.Geometry] = None,
+      geometry: Optional[Union[ee.Geometry, types.types.BBox]] = None,


Should it just be just types and not types.types?

alxmrs · 2024-02-18T03:24:38Z

xee/ext.py

      x_min_0, y_min_0, x_max_0, y_max_0 = _ee_bounds_to_bounds(
          self.get_info['bounds']
      )
+    elif isinstance(geometry, Union[List, Tuple, np.ndarray]):


Does typing.Sequence work here?

Consider adding a check for length of the sequence, too.

Ah! You have one :)

xee/ext.py

alxmrs · 2024-02-18T04:14:09Z

xee/ext_integration_test.py

+    xee_dataset = xr.open_dataset(
+        ee.ImageCollection(ic),
+        engine='ee',
+        geometry=tuple(raster.rio.bounds()),


alxmrs · 2024-02-18T04:15:03Z

xee/ext_integration_test.py

+        ee.ImageCollection(ic),
+        engine='ee',
+        geometry=tuple(raster.rio.bounds()),
+        scale=raster.rio.resolution()[0],


Related: should we let users pass in x and y scale?

(Is this possible somehow today, eg via proj?)

Yes, it is. Great call! I added this to the integration test in a2b3c55

alxmrs

LGTM. Added a couple of nits, but overall I really like this improvement. I think CI needs to pass for this to be merged.

alxmrs · 2024-02-20T03:17:04Z

xee/ext.py

@@ -139,7 +139,7 @@ def open(
      crs: Optional[str] = None,
      scale: Optional[float] = None,
      projection: Optional[ee.Projection] = None,
-      geometry: Optional[ee.Geometry] = None,
+      geometry: Optional[Union[ee.Geometry, types.BBox]] = None,


Consider just using a union with None. Optional is like a Union, so this is somewhat redundant.

Maybe you misread or I am misunderstanding. It sounds like you think I put Optional[Union[ee.Geometry, None]] in which case, yes, agreed--redundant, but I added types.BBox. I actually am not sure if the type types.BBox will display as tuple[float, float, float, float] in jupyter or something and I am somewhat inclined to change to Optional[Union[ee.Geometry, Tuple[float, float, float, float]] = None to make it as clear as possible.

Implemented in 4c0dd6b

Union[ee.Geometry, types.BBox, None] is equivalent to Optional[Union[ee.Geometry, types.BBox]] and in my opinion a bit cleaner.

Sorry for being unclear. I'm referring to the definition of Optional: https://docs.python.org/3/library/typing.html#typing.Optional

I think I was confused, but your preference is clear now! Thanks. Updated in 23ea0c9

alxmrs · 2024-02-20T03:17:57Z

xee/ext.py

      x_min_0, y_min_0, x_max_0, y_max_0 = _ee_bounds_to_bounds(
          self.get_info['bounds']
      )
+    elif isinstance(geometry, Union[List, Tuple, np.ndarray, Sequence]):


Sorry, I meant can sequence replace the rest.

One thing to consider is that sequence will let in a simple string e.g. isinstance("hotdog", Sequence) evaluates to True. Still want me to drop all types in favor of Sequence alone? There is a part of me that is also weirded out by even using np.ndarray since that could be an incorrect shape that still evaluates to length four.

I think I would like to be more stringent with types and changes this to only Union[List, Tuple].

Implemented in 4c0dd6b

Thanks for checking. SGTM.

alxmrs · 2024-02-20T03:19:47Z

xee/ext_integration_test.py

+        engine='ee',
+        geometry=tuple(raster.rio.bounds()),
+        projection=ee.Projection(
+            crs=str(raster.rio.crs), transform=raster.rio.transform()[:6]


ljstrnadiii · 2024-02-20T15:20:24Z

@alxmrs thanks for another review! I am going to simplify types as I mentioned in comments and add a small demo in the readme. As far as CI, I think auth is failing because my user name does have the credentials? I am not sure if this is on my end or yours? I am guessing an author/google employee may need to trigger CI for auth to work correctly?

alxmrs · 2024-02-22T08:08:25Z

I think @jdbcode or @naschmitz can help with CI. I’m not familiar with how it’s set up.

ljstrnadiii · 2024-02-22T14:06:34Z

@jdbcode or @naschmitz any tips? The tests workflow fails at authentication.

ljstrnadiii · 2024-03-02T18:04:34Z

@jdbcode @naschmitz kindly bumping

naschmitz · 2024-03-13T16:35:50Z

Sorry for the delay. We're extremely understaffed right now. I'm going to take a look at your PR now.

naschmitz

Looks good to me! I'll approve once the changes are made.

naschmitz · 2024-03-13T16:37:58Z

xee/ext.py

-      geometry: Optional[ee.Geometry] = None,
+      geometry: Optional[
+          Union[ee.Geometry, Tuple[float, float, float, float]]
+      ] = None,


Could you update the type annotation to the following. Geometry | Tuple | None is preferred over Optional[Geometry | Tuple].

Union[ee.Geometry, Tuple[float, float, float, float], None]

Here and other places in the PR.

implemented here b640d95

naschmitz · 2024-03-13T16:38:42Z

xee/ext.py

+      try:
+        x_min_0, y_min_0, x_max_0, y_max_0 = self.crs.area_of_use.bounds
+      except AttributeError:
+        # `area_of_use` is probable `None`. Parse the geometry from the first


Nit: probably

implemented here b640d95

naschmitz · 2024-03-13T16:41:25Z

xee/ext.py

      x_min_0, y_min_0, x_max_0, y_max_0 = _ee_bounds_to_bounds(
          self.get_info['bounds']
      )
+    elif isinstance(geometry, Union[List, Tuple]):


You should be able to check against Sequence here instead.

That is fine, but a string is also sequence and this would catch and raise in that case. Updated anyways.

implemented here b640d95

naschmitz · 2024-03-13T16:43:32Z

xee/ext.py

@@ -231,19 +235,31 @@ def __init__(
    # Parse the dataset bounds from the native projection (either from the CRS


Consider breaking this out into a separate function. This logic is much more complex now.

Looks like the only input is geometry. Return tuple should be a 4-length tuple.

added in b640d95

ljstrnadiii · 2024-03-16T15:25:35Z

@naschmitz thanks for the review and no worries on the timeline. I'm just stoked to get it in as this is basically my most substantial OSS contribution to date after 6 years of startup life.

Tests pass locally btw! Thanks again and let me know if there is anything else you would like to see 🙏

ljstrnadiii · 2024-03-21T15:24:25Z

Any idea what is up with CI test step authentication?

ljstrnadiii added 3 commits February 13, 2024 02:57

add option for using double precision coords

0b78180

add match xarray support

6983737

run pyint

7efe992

ljstrnadiii commented Feb 14, 2024

View reviewed changes

ljstrnadiii added 4 commits February 14, 2024 18:51

revert any credential changes

e891f0a

Revert "run pyint"

5e19d99

This reverts commit 7efe992.

run pyint

f83c749

formatted change

af97750

alxmrs reviewed Feb 15, 2024

View reviewed changes

alxmrs reviewed Feb 16, 2024

View reviewed changes

ljstrnadiii added 2 commits February 17, 2024 15:17

checkout ext.py from main + create failing integration test

8373171

add support for bbox in geometry + demo passing integration test

f6048f6

ljstrnadiii changed the title ~~feat: match_xarray and use_coords_double_precision~~ feat: support bbox in in geometry Feb 17, 2024

update integation test to pass bounds

00b34e7

ljstrnadiii requested a review from alxmrs February 17, 2024 16:03

alxmrs reviewed Feb 18, 2024

View reviewed changes

xee/ext.py Show resolved Hide resolved

alxmrs reviewed Feb 18, 2024

View reviewed changes

ljstrnadiii added 2 commits February 18, 2024 17:11

demonstrate xy scale control through projection in test

a2b3c55

nits

4db0887

ljstrnadiii requested a review from alxmrs February 20, 2024 01:53

alxmrs approved these changes Feb 20, 2024

View reviewed changes

ljstrnadiii and others added 4 commits February 20, 2024 15:43

narrow supported types

4c0dd6b

add readme example

a766527

Merge branch 'main' into float64_coords

07fb92d

typing nit

23ea0c9

naschmitz reviewed Mar 13, 2024

View reviewed changes

naschmitz assigned naschmitz and ljstrnadiii and unassigned ljstrnadiii and naschmitz Mar 13, 2024

nits + pull out logic + typing

b640d95

ljstrnadiii requested a review from naschmitz March 16, 2024 15:23

naschmitz approved these changes Mar 19, 2024

View reviewed changes

naschmitz added the pull ready label Mar 19, 2024

copybara-service bot merged commit 5fe4395 into google:main Mar 21, 2024
6 of 11 checks passed

		@@ -41,17 +41,14 @@
		]


		def _read_identity_pool_creds() -> identity_pool.Credentials:

		@@ -231,19 +235,31 @@ def __init__(
		# Parse the dataset bounds from the native projection (either from the CRS

feat: support bbox in in geometry #138

feat: support bbox in in geometry #138

Conversation

ljstrnadiii commented Feb 13, 2024 • edited Loading

Motivating Use Case of match_xarray:

Motivating Use Case of use_coords_double_precision:

ljstrnadiii commented Feb 13, 2024 • edited Loading

ljstrnadiii commented Feb 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljstrnadiii commented Feb 14, 2024

ljstrnadiii commented Feb 14, 2024

jdbcode commented Feb 14, 2024

Choose a reason for hiding this comment

alxmrs left a comment

Choose a reason for hiding this comment

ljstrnadiii commented Feb 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxmrs commented Feb 16, 2024

ljstrnadiii commented Feb 16, 2024 • edited Loading

alxmrs commented Feb 17, 2024 via email

ljstrnadiii commented Feb 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxmrs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljstrnadiii commented Feb 20, 2024 • edited Loading

alxmrs commented Feb 22, 2024

ljstrnadiii commented Feb 22, 2024

ljstrnadiii commented Mar 2, 2024

naschmitz commented Mar 13, 2024

naschmitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljstrnadiii commented Mar 16, 2024

ljstrnadiii commented Mar 21, 2024

ljstrnadiii commented Feb 13, 2024 •

edited

Loading

Motivating Use Case of `match_xarray`:

Motivating Use Case of `use_coords_double_precision`:

ljstrnadiii commented Feb 13, 2024 •

edited

Loading

ljstrnadiii commented Feb 16, 2024 •

edited

Loading

ljstrnadiii commented Feb 20, 2024 •

edited

Loading