Convert AreaDefinitions to odc geoboxes #545

BENR0 · 2023-10-10T14:01:44Z

Convert AreaDefinitions to odc geoboxes (https://odc-geo.readthedocs.io/en/latest/).

Tests added
Tests passed
Passes git diff origin/main **/*py | flake8 --diff
Fully documented

… feat_to_odc_geobox

codecov · 2023-10-10T14:10:14Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (e82bf47) 94.06% compared to head (e560f1c) 94.05%.

Files	Patch %	Lines
pyresample/geometry.py	75.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #545      +/-   ##
==========================================
- Coverage   94.06%   94.05%   -0.01%     
==========================================
  Files          85       85              
  Lines       13250    13276      +26     
==========================================
+ Hits        12463    12487      +24     
- Misses        787      789       +2

Flag	Coverage Δ
unittests	`94.05% <92.30%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

coveralls · 2023-10-10T14:28:04Z

coverage: 93.642% (-0.002%) from 93.644%
when pulling e560f1c on BENR0:feat_to_odc_geobox
into e82bf47 on pytroll:main.

djhoese

Love it! I had some requests, but otherwise I think this is an obvious addition. I'd be curious what @ghiggi's opinion is on this as I'm wondering if we could essentially drop our existing boundary stuff (eventually). That assumes I understand what odc-geo's boxes can do which I may not have a great understanding.

djhoese · 2023-10-10T18:24:42Z

pyresample/geometry.py

+            raise ModuleNotFoundError("Please install 'odc-geo' to use this method.")
+
+        return GeoBox.from_bbox(bbox=self.area_extent, crs=self.crs,
+                                resolution=np.mean([self.pixel_size_x, self.pixel_size_y]), tight=True)


I think resolution can be a 2 element pair using the Resolution class:

https://odc-geo.readthedocs.io/en/latest/_api/odc.geo.Resolution.html#odc.geo.Resolution

I know I can't really remember why I didn't use it but will check and change it eventually.

I changed it to use the Resolution class. One thing to note is that by default if Resolution is given one value it is assumed that the second (y) value is -x. Because in Pyresample resolution values are always positive I added a minus sign in front of the y value to keep the behavior I initially implemented. In the GeoBox this affects where the "anchor" point (i.e. from where the raster is indexed). This is mostly visible in the affine transformation of the GeoBox. You can also see this in the representation of the GeoBox as seen below (top is the implemented behavior, bottom is the behavior when both resolution values are positive; look at the small dot on the boundary).

djhoese · 2023-10-10T18:25:55Z

setup.py

+    all_extras.extend(extra_deps)
+extras_require['all'] = list(set(all_extras))
+
+setup_requires = ['numpy>=1.10.0', 'cython']


This isn't actually used anywhere, plus this kind of thing (setup_requires) should be defined in the pyproject.toml build section.

Good point. I have to admit I just copied it to get the different extra install options working.

djhoese · 2023-10-10T18:26:27Z

pyresample/test/test_geometry/test_area.py

@@ -403,6 +403,35 @@ def test_cartopy_crs_latlon_bounds(self, create_test_area):
        latlong_crs = area_def.to_cartopy_crs()
        np.testing.assert_allclose(latlong_crs.bounds, [-180, 180, -90, 90])

+    def test_to_odc_geobox(self, stere_area, create_test_area):


Can we xfail or skip if odc is missing? I think pytest has a decorator specifically for skipping if import fails.

That's a good Idea. I will also add a test for missing odc-geosince that is not tested yet.

And thanks for using the create_test_area fixture. It makes my work on Pyresample 2.0 much easier.

I added a test for the case where odc-geo is not installed and changed the setup to include it as a requirement for testing. I thought about it and I think the test should not be skipped if odc-geo is not installed instead odc-geo should be required for testing as it is now.

BENR0 · 2023-10-10T19:01:54Z

Glad that you like it was not sure if it is a valuable addition. I just used it because I was to lazy to create Affine transformations from scratch and was thinking that maybe I can test some stuff for rerprojections and maybe for the html representation.

ghiggi · 2023-10-10T19:50:48Z

Cool feature @BENR0. I never used the odc-geo tool. But from a quick look @djhoese:

does not support swath definition data
I am not sure it currently support areas crossing the antimeridian (@BENR0 could you test the result with GOES-17 AreaDef? area_def = satpy.resample.get_area_def("goes_west_abi_f_1km")
The implemented geometry operations are implemented wrapping shapely ... so classical problems ...
--> We could maybe use the odc-geo for the get_bbox_lonlats of AreaDefinition. But it's worth?

BTW I just opened the PR #546 to streamline the boundary extraction

djhoese · 2023-10-11T14:47:18Z

does not support swath definition data

Sure, but I think that's the point. They're projected bounding boxes.

I am not sure it currently support areas crossing the antimeridian (@BENR0 could you test the result with GOES-17 AreaDef? area_def = satpy.resample.get_area_def("goes_west_abi_f_1km")
The implemented geometry operations are implemented wrapping shapely ... so classical problems ...
--> We could maybe use the odc-geo for the get_bbox_lonlats of AreaDefinition. But it's worth?

I would suspect they don't currently support the anti-meridian as that is technically not a valid contiguous space on any CRS. If you have data that crosses the anti-meridian of a CRS then your data can't be represented correctly on that CRS. This is strict and difficult to reconcile in real world applications with data crossing the poles, but it is technically true. I know you have workarounds in your other PRs since we do end up having to deal with it, but I just wanted to point this out.

As for using odc-geo in other methods of the geometry objects: no thank you. Not yet. Not unless it is a clear win for doing calculations. No need for the extra dependency.

mraspaud

LGTM

djhoese · 2023-12-14T04:06:31Z

Not that I expected this to happen in this PR, but we really need to come up with a new pattern for conversion of areas to other things. I'm not sure dumping it all in geometry.py is a good idea.

BENR0 and others added 9 commits August 4, 2023 11:51

feat: add to_odc_geobox method

8f5fe3b

Add: odc-geo to extras list.

4b02a02

fix: geobox pixel snapping.

bb4d79c

test: add geobox conversion test.

5e600a1

refactor: add exception for none installed odc-geo.

fcf296b

fix: odc-geo not in ci environment dependency list.

ec53f26

Merge branch 'main' into feat_to_odc_geobox

9381bff

Merge branch 'main' into feat_to_odc_geobox

87b2bf1

Merge branch 'feat_to_odc_geobox' of github.com:BENR0/pyresample into…

acff8b9

… feat_to_odc_geobox

djhoese requested changes Oct 10, 2023

View reviewed changes

Benjamin Rösner and others added 5 commits October 24, 2023 13:23

refactor: remove unnecessary setup_requires.

84a6deb

fix: odc-geo not in test_requires list.

28c6f80

refactor: use odc Resolution class.

c043b97

refactor: move odc-geo import to top, add test for missing odc-geo.

09c35fd

Merge branch 'main' into feat_to_odc_geobox

e560f1c

mraspaud approved these changes Dec 13, 2023

View reviewed changes

mraspaud merged commit 522691d into pytroll:main Dec 13, 2023
22 of 27 checks passed

mraspaud assigned BENR0 Dec 13, 2023

mraspaud added the enhancement label Dec 13, 2023

BENR0 deleted the feat_to_odc_geobox branch April 4, 2024 06:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert AreaDefinitions to odc geoboxes #545

Convert AreaDefinitions to odc geoboxes #545

BENR0 commented Oct 10, 2023

codecov bot commented Oct 10, 2023 •

edited

Loading

coveralls commented Oct 10, 2023 •

edited

Loading

djhoese left a comment

djhoese Oct 10, 2023

BENR0 Oct 10, 2023

BENR0 Nov 7, 2023

djhoese Oct 10, 2023

BENR0 Oct 10, 2023

djhoese Oct 10, 2023

BENR0 Oct 10, 2023

djhoese Oct 11, 2023

BENR0 Nov 7, 2023

BENR0 commented Oct 10, 2023

ghiggi commented Oct 10, 2023 •

edited

Loading

djhoese commented Oct 11, 2023

mraspaud left a comment

djhoese commented Dec 14, 2023

Convert AreaDefinitions to odc geoboxes #545

Convert AreaDefinitions to odc geoboxes #545

Conversation

BENR0 commented Oct 10, 2023

codecov bot commented Oct 10, 2023 • edited Loading

Codecov Report

coveralls commented Oct 10, 2023 • edited Loading

djhoese 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

BENR0 commented Oct 10, 2023

ghiggi commented Oct 10, 2023 • edited Loading

djhoese commented Oct 11, 2023

mraspaud left a comment

Choose a reason for hiding this comment

djhoese commented Dec 14, 2023

codecov bot commented Oct 10, 2023 •

edited

Loading

coveralls commented Oct 10, 2023 •

edited

Loading

ghiggi commented Oct 10, 2023 •

edited

Loading