Add optional metadata to Pyresample 2.0 AreaDefinition #464

djhoese · 2022-10-28T21:01:42Z

As part of one of my projects it has been requested that AreaDefinition's officially support a container of metadata. This is inline with what we had planned for Pyresample 2.0. This PR is the initial support of this functionality.

Note: At the time of writing this is mostly just a refactor of pyresample's future geometry objects to prepare for this functionality.

codecov · 2022-10-28T21:10:35Z

Codecov Report

Merging #464 (be2bf83) into main (b8ecc71) will decrease coverage by 0.07%.
The diff coverage is 99.15%.

@@            Coverage Diff             @@
##             main     #464      +/-   ##
==========================================
- Coverage   94.33%   94.27%   -0.07%     
==========================================
  Files          74       78       +4     
  Lines       12947    12910      -37     
==========================================
- Hits        12214    12171      -43     
- Misses        733      739       +6

Flag	Coverage Δ
unittests	`94.27% <99.15%> (-0.07%)`	⬇️

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

Impacted Files	Coverage Δ
pyresample/area_config.py	`90.65% <ø> (ø)`
pyresample/ewa/ewa.py	`75.86% <ø> (ø)`
pyresample/test/test_resamplers/test_nearest.py	`100.00% <ø> (ø)`
pyresample/test/test_geometry_legacy.py	`97.80% <97.80%> (ø)`
pyresample/test/test_geometry/test_area.py	`99.18% <99.18%> (ø)`
pyresample/future/geometry/__init__.py	`100.00% <100.00%> (ø)`
pyresample/future/geometry/area.py	`100.00% <100.00%> (ø)`
pyresample/future/geometry/base.py	`100.00% <100.00%> (ø)`
pyresample/test/conftest.py	`98.40% <100.00%> (+0.38%)`	⬆️
pyresample/test/test_geometry/__init__.py	`100.00% <100.00%> (ø)`
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

coveralls · 2022-10-28T21:20:42Z

Coverage: 93.799% (-0.06%) from 93.862% when pulling be2bf83 on djhoese:feature-area-metadata into b8ecc71 on pytroll:main.

mraspaud

Thanks for starting this! a few questions

mraspaud · 2022-11-07T14:37:43Z

pyresample/future/geometry/area.py

+)
+
+
+class AreaDefinition(LegacyAreaDefinition):


Do we want the new area def to inherit all the methods of the legacy one, or do we want to split responsibility a bit?

Yeah I struggled with this decision. Initially this class in the future subpackage didn't have anything custom about it so it was just a basic import. I think for ease of transitioning to future we might want it to be a subclass so that isinstance checks still work? Or maybe usages of stuff like that should be fixed to be more duck-typing-friendly?

I vote for more duck typing.

Please read my other comments. That is the plan, but I don't want to throw that all into one PR and I also don't want to deal with all the tests failing because I didn't implement one of 50 methods on the legacy AreaDefinition class. The plan is to do this work in one of the next 2 or 3 PRs.

pyresample/test/test_geometry/test_area.py

Co-authored-by: Martin Raspaud <martin.raspaud@smhi.se>

djhoese · 2022-11-24T15:27:40Z

I think my biggest TODO on this is to move the area ID to an optional "name" metadata key in .attrs.

Includes major rewrite of future area tests to test both legacy and future area class with pytest fixtures

djhoese · 2023-01-27T19:25:19Z

@mraspaud So my last commits add a new pytest fixture which returns a function that automatically handles keyword argument rearranging and defaulting so the tests in pyresample/test/test_geometry/test_area.py now test both the legacy AreaDefinition class and the new/future one. In these last commits I also removed area_id as a required parameter and made attrs={"name": "some name"} an optional special metadata value along with description.

Some things I thought of or noticed while doing this and/or things that I see as needing to be done. @pnuu @gerritholl @ghiggi I'm curious on your opinions about this as well.

I'd like to convert width and height into a single shape argument.
I'd like to add a "feature flag" to pyresample, probably with a donfig config object, that will allow users to say "all utility methods, use the new class". I need this to complete the test_area.py code testing both classes as it often uses the utility create_area_def which always makes the old area definition classes.
After the new tests are done, I'd like to remove the old area tests module and only use the new one. The new tests aren't named in a "future" way at all (test/test_geometry/test_area.py) so it should be clear to contributors that the AreaDefinition tests are in the new tests module. To really do this I may need to also copy over a lot of SwathDefinition tests which may not be fun, but is going to be needed eventually so 🤷‍♂️.

All of these things deserve their own PR in my opinion. It looks like I still need to fix some tests, but otherwise I think I'd like to merge this. Since it only affects the future interfaces merging this shouldn't break anything.

djhoese · 2023-01-27T21:46:40Z

Unstable CI is failing because matplotlib needs importlib-resources but we explicitly don't install dependencies for unstable packages.

Edit: If we bumped to Python 3.10 it would work fine.

ghiggi

I added some inline comments on the current PR.

Other minor comments/ideas related to AreaDefinition attributes and properties:

For AreaDefinition, many methods are not clear if they return data in xy or lat/lon; the pixel centroid or the outermost pixel corner. Maybe would be worth adding a common suffix (i.e. _ll) when they return lat/lons, and otherwise expect they return xy coords. This will enhance code readability.
Example:
- area_def.corners currently returns the centroid lat/lon coords in the very old spherical object
- area_def.outer_boundary_corners currently returns the corners lat/lon coords in the very old spherical object
  --> Possible new methods: corners_centroids, corners, corners_centroids_ll, corners_ll ... or see next bullet point
Add utility properties like upper_left_corner_ll, upper_left_centroid_ll,
A possible pattern: <lower/upper>_<right/left>_<''/'ll'> (or bottom instead of lower).
Currently the upper_left_corner is provided by upper_left_extent
Add xy_bbox and ll_bbox properties for consistency with satpy? Equivalent to area_extent and area_extent_ll
Current area_extent_<> and <>_bbox follow the (xmin, ymin, xmax, ymax) convention. More precisely the area_extent is defined as (lower_left_x, lower_left_y, upper_right_x, upper_right_y).
- This is also the convention adopted by rasterio/rioxarray/shapely object bounds method.
  --> Maybe we could add a bounds property?
- When retrieving the area_extent_ll (in lat/lon) the lower-left and upper-right corners do not necessarily currently define the outermost lat/lon coordinates of the area. Maybe this method should be changed! And maybe we could also add a bounds_ll method.
- Additionally, the area_extent convention differs from the i.e. matplotlib cartopy and matplotlib extent which is defined as (xmin, xmax, ymin, ymax)
  --> Maybe we could add a <mpl/cartopy>_area_extent_<''/ll> property?
For GEO AreaDef, all the above _ll methods currently return np.nan/np.inf values. Maybe ad-hoc processing using get_geostationary_bounding_box_in_lonlats could be implemented for area_extent_ll, but the corners would remain undefined ! Maybe would be worth raising an error to avoid downstream bugs !

ghiggi · 2023-01-27T21:06:41Z

pyresample/future/geometry/area.py

+            y dimension in number of pixels, aka number of grid rows
+        size (int):
+            Number of points in grid
+        area_extent_ll (tuple):


In the attributes, it should be listed also the area_extent .

ghiggi · 2023-01-27T21:07:53Z

pyresample/future/geometry/area.py

+        height:
+            y dimension in number of pixels, aka number of grid rows
+        area_extent:
+            Area extent as a list (lower_left_x, lower_left_y, upper_right_x, upper_right_y)


Suggested change

Area extent as a list (lower_left_x, lower_left_y, upper_right_x, upper_right_y)

Area extent (in projection coordinates) provided as a list (lower_left_x, lower_left_y, upper_right_x, upper_right_y)

ghiggi · 2023-01-27T21:20:52Z

pyresample/future/geometry/area.py

+            Pixel width in projection units
+        pixel_size_y (float):
+            Pixel height in projection units
+        upper_left_extent (tuple):


upper_left_extent is a misleading name. I would rather call it upper_left_corner. It's used only in AreaDefinition.from_ul_corner btw

extent is the extreme border in all extent definitions I've seen, also outside of Pytroll, so I don't see that as very misleading.

ghiggi · 2023-01-27T21:21:58Z

pyresample/future/geometry/area.py

+            Pixel height in projection units
+        upper_left_extent (tuple):
+            Coordinates (x, y) of upper left corner of upper left pixel in projection units
+        pixel_upper_left (tuple):


Just a thought: maybe pixel_upper_left could be renamed upper_left_centroid (see previous comment)

djhoese · 2023-01-28T01:50:59Z

Thanks for all the feedback @ghiggi, but I think there's been a misunderstanding. The tests added here (with a few exception for the new metadata handling) are all copies from the original AreaDefinition tests. None of these coordinate features/names are new/different. I think we should maybe move the discussion of changes to the AreaDefinition interface somewhere else...I thought I had an issue for that 🤔

I have my own ideas for changing interfaces and some of us have discussed them in the past. For example, put all coordinate operations (ex. get_lonlats) in a separate coordinate handling class.

Looks like there are no "here's all the area stuff we want to change" issues. We have these for v2.0: https://github.com/pytroll/pyresample/issues?q=is%3Aopen+is%3Aissue+milestone%3Av2.0

djhoese · 2023-01-31T21:09:45Z

@ghiggi do you have any additional comments about the stuff I did in this PR? Mainly my comment just before your last comment, the idea of metadata in the AreaDefinition (in pyresample 2.0), or the pytest fixture I'm using to test both legacy areas and new areas?

ghiggi · 2023-02-01T20:45:57Z

Sorry for the very late reply (and the misunderstanding) @djhoese ... I am having super busy days.
I have no additional comments ;)

mraspaud

Thanks for the PR. Some comments inline

mraspaud · 2023-02-06T16:19:06Z

pyresample/future/geometry/area.py

+)
+
+
+class AreaDefinition(LegacyAreaDefinition):


I vote for more duck typing.

pyresample/test/conftest.py

pyresample/future/geometry/area.py

mraspaud · 2023-02-06T16:46:53Z

pyresample/test/test_geometry/test_area.py

+            area_def.area_extent = (-1000000, -900000, 1000000, 1500000)
+
+
+class TestAreaDefinitionMetadata:


I'm not sure about the name here. At least, should this class and all that is future-specific be moved to a future test file?

I thought about this a lot when I started this PR and I think I still agree with past Dave. Or at least I'm 50/50 on it so I can't say I feel that strongly. The original plan (and how it is being implemented now) is that this module will test legacy and future implementations and the legacy test module will eventually be removed. So I didn't want to confuse contributions for the legacy area definition by telling people "I know you want to put the tests in test/, but they should actually go in test/future/ even though that's not where your changes went.

mraspaud · 2023-02-06T16:50:14Z

pyresample/test/test_geometry/test_area.py

@@ -0,0 +1,2003 @@
+# Copyright (C) 2010-2022 Pyresample developers


Feels like this is a copy of existing tests? Is this really needed? can't we have just one version of this?

"one version" of what? Of the AreaDefinition tests? Please read my comments with my questions and my plans to remove the legacy-only tests right after this PR is merged.

...and yes this copyright is here like this because these tests were copied from the original/legacy geometry tests so I guess the copyright still stands?

yeah, this wasn't about the copyright :)
Ok, so I think it's a good idea to have have tests for both legacy and future in one module. But I would like to avoid being in a limbo for a while where we have duplicated tests. Do you think it's possible to remove the legacy tests already in this PR?

If that's really what you want. The idea was that I was going to make a ton of PRs right away after this merge a week or so ago when I had time. Now I'm not so sure how fast I can do that so I guess I can put it here.

djhoese · 2023-02-08T19:17:07Z

Strange. Github complained about merge conflicts but when I merged it locally it did it perfectly fine. 🤷‍♂️

Now to start on converting all the old tests...

djhoese · 2023-02-10T02:58:59Z

@mraspaud Ok I think this is ready from the "no duplicate tests" point of view. The tests are much more organized and are all pytest-based now. The remaining tests in the legacy module are for classes that don't currently have a future-pyresample version.

mraspaud

I think we're good for now, let's merge this and see what happens

Start refactoring future geometry AreaDefinition to support metadata

97660d1

djhoese added the enhancement label Oct 28, 2022

djhoese added this to the v2.0 milestone Oct 28, 2022

djhoese added 2 commits October 29, 2022 07:10

Fix a few docstrings

d350168

Fix docstring references to AreaDefinition

2fcabc9

djhoese self-assigned this Oct 31, 2022

Add .attrs to pyresample 2.0 AreaDefinition

545c5c4

djhoese marked this pull request as ready for review October 31, 2022 17:52

mraspaud reviewed Nov 7, 2022

View reviewed changes

djhoese and others added 2 commits November 7, 2022 08:48

Update copyright year in pyresample/test/test_geometry/test_area.py

2869938

Co-authored-by: Martin Raspaud <martin.raspaud@smhi.se>

Rewrite future area tests to use pytest

edebe75

djhoese added 3 commits January 27, 2023 10:23

Merge branch 'main' into feature-area-metadata

2a1afb7

Port test changes from legacy tests to future area tests

c4cc9bd

Remove 'area_id' as required metadata for future AreaDefinition class

da6abd6

Includes major rewrite of future area tests to test both legacy and future area class with pytest fixtures

Fix future resampler tests for changes to the future AreaDefinition

d624e3f

ghiggi reviewed Jan 27, 2023

View reviewed changes

djhoese mentioned this pull request Jan 31, 2023

Deprecate AreaDefinition 'rotation' argument #496

Merged

5 tasks

mraspaud reviewed Feb 6, 2023

View reviewed changes

Merge branch 'main' into feature-area-metadata

3d3c842

djhoese added 2 commits February 8, 2023 14:04

Port new geostationary bbox tests to test_area.py module

4e90852

Move some swath tests to test_geometry/test_swath.py

1135ff5

djhoese added 12 commits February 8, 2023 15:28

Consolidate swath hashing tests and other import cleanups

f528ca8

Consolidate area equality tests

d1c40c6

More area test cleanup

97dbf70

More area test cleanup (get_proj_coords)

f90e95c

Add deprecation note to test_geometry_legacy.py docstring

1e2f40d

More area test cleanup

d22502f

More area test cleanup

7d8b2b6

More area test cleanup

22c3964

Move primary swath test class to test_geometry/test_swath.py

2fb0611

More swath test cleanup

8b2b32b

More area test cleanup

88eeb62

Move boundary tests to geometry specific test modules

be2bf83

mraspaud approved these changes Feb 13, 2023

View reviewed changes

mraspaud merged commit f770984 into pytroll:main Feb 13, 2023

djhoese deleted the feature-area-metadata branch February 28, 2023 20:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional metadata to Pyresample 2.0 AreaDefinition #464

Add optional metadata to Pyresample 2.0 AreaDefinition #464

djhoese commented Oct 28, 2022

codecov bot commented Oct 28, 2022 •

edited

Loading

coveralls commented Oct 28, 2022 •

edited

Loading

mraspaud left a comment

mraspaud Nov 7, 2022

djhoese Nov 7, 2022

mraspaud Feb 6, 2023

djhoese Feb 6, 2023

djhoese commented Nov 24, 2022

djhoese commented Jan 27, 2023

djhoese commented Jan 27, 2023 •

edited

Loading

ghiggi left a comment •

edited

Loading

ghiggi Jan 27, 2023

ghiggi Jan 27, 2023

ghiggi Jan 27, 2023

pnuu Jan 27, 2023

ghiggi Jan 27, 2023

djhoese commented Jan 28, 2023

djhoese commented Jan 31, 2023

ghiggi commented Feb 1, 2023

mraspaud left a comment

mraspaud Feb 6, 2023

mraspaud Feb 6, 2023

djhoese Feb 6, 2023 •

edited

Loading

mraspaud Feb 6, 2023

djhoese Feb 6, 2023

djhoese Feb 6, 2023

mraspaud Feb 7, 2023

djhoese Feb 7, 2023

djhoese commented Feb 8, 2023

djhoese commented Feb 10, 2023

mraspaud left a comment

	Area extent as a list (lower_left_x, lower_left_y, upper_right_x, upper_right_y)
	Area extent (in projection coordinates) provided as a list (lower_left_x, lower_left_y, upper_right_x, upper_right_y)

		area_def.area_extent = (-1000000, -900000, 1000000, 1500000)


		class TestAreaDefinitionMetadata:

		@@ -0,0 +1,2003 @@
		# Copyright (C) 2010-2022 Pyresample developers

Add optional metadata to Pyresample 2.0 AreaDefinition #464

Add optional metadata to Pyresample 2.0 AreaDefinition #464

Conversation

djhoese commented Oct 28, 2022

codecov bot commented Oct 28, 2022 • edited Loading

Codecov Report

coveralls commented Oct 28, 2022 • edited Loading

mraspaud 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

djhoese commented Nov 24, 2022

djhoese commented Jan 27, 2023

djhoese commented Jan 27, 2023 • edited Loading

ghiggi left a comment • edited Loading

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

djhoese commented Jan 28, 2023

djhoese commented Jan 31, 2023

ghiggi commented Feb 1, 2023

mraspaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djhoese Feb 6, 2023 • edited Loading

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

djhoese commented Feb 8, 2023

djhoese commented Feb 10, 2023

mraspaud left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 28, 2022 •

edited

Loading

coveralls commented Oct 28, 2022 •

edited

Loading

djhoese commented Jan 27, 2023 •

edited

Loading

ghiggi left a comment •

edited

Loading

djhoese Feb 6, 2023 •

edited

Loading