Refactor area boundary sides retrieval with `_geographic_sides` and `_projection_sides` methods #566

ghiggi · 2023-12-08T14:00:51Z

This PR introduces the methods:

_get_geographic_sides
_get_projection_sides

These two methods allow to retrieve the boundary sides for whatever pyresample area, whether is geostationary or not.
In a future PR, these two methods will also include the logic to retrieve the boundary of AreaDefinition with out-of-Earth coordinates.

This PR:

does not impact get_area_slices method
does not impact the AreaSlicer and SwathSlicer class yet
does not fix GEO boundary definition issues
does not fix boundary retrieval for out-of-earth disk areas

BUT :

_get_geographic_sides is called within get_bbox_lonlats
So from now on, get_bbox_lonlats returns non-Inf sides vertices for geostationary area !
Since get_edge_lonlats and boundary() depends on get_bbox_lonlats, they now also returns non-Inf GEO vertices
Note that the order of boundary vertices of get_bbox_lonlats has not yet be fixed to be correct.

_get_geographic_sides

it's required in the coming PR to create the SphericalBoundary class
get_bbox_lonlats is gonna be deprecated in upcoming PRs

_get_projection_sides

is not yet used and tested in this PR
it's required for the coming PR to create the SphericalBoundary and PlanarBoundary class
it's required for the coming PR to deprecate usage of get_edge_bbox_in_projection_coordinates in SwathSlicer/AreaSlicer
it's required for the coming PR to deprecate usage of get_geostationary_bounding_box_in_proj_coords in AreaSlicer

djhoese · 2023-12-08T16:16:02Z

pyresample/gradient/__init__.py

+            return False
+        try:
+            poly = get_polygon(self.prj, geo_def)
+        except Exception:


Can we use a more specific exception here?

Done. It can arise from

NotImplementedError in get_geo_bounding_box (totally out of Earth disk)

ValueError when not valid boundary coordinates (all nan or inf)

ValueError when not at least 4 valid vertices can be retrieved for boundary sides creation.

djhoese · 2023-12-08T16:38:07Z

pyresample/geometry.py

-            warnings.warn("The `frequency` argument is pending deprecation, use `vertices_per_side` instead",
-                          PendingDeprecationWarning, stacklevel=2)
-        vertices_per_side = vertices_per_side or frequency
-        x, y = self._get_bbox_elements(self.get_proj_coords, vertices_per_side)


My only guess is to merge with main and see if it behaves better.

codecov · 2023-12-08T17:01:56Z

Codecov Report

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

Comparison is base (c240c04) 94.09% compared to head (aee90a1) 94.06%.

Files	Patch %	Lines
pyresample/geometry.py	81.39%	8 Missing ⚠️
pyresample/gradient/__init__.py	81.81%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #566      +/-   ##
==========================================
- Coverage   94.09%   94.06%   -0.04%     
==========================================
  Files          85       85              
  Lines       13235    13250      +15     
==========================================
+ Hits        12453    12463      +10     
- Misses        782      787       +5

Flag	Coverage Δ
unittests	`94.06% <84.12%> (-0.04%)`	⬇️

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-12-08T17:14:03Z

coverage: 93.644% (-0.03%) from 93.675%
when pulling aee90a1 on ghiggi:refactor-geometry-sides-creation
into c240c04 on pytroll:main.

ghiggi · 2023-12-08T17:16:51Z

@djhoese ok now it works. Ready for review and eventually merge ;)

djhoese · 2023-12-08T17:19:43Z

pyresample/geometry.py

@@ -1563,8 +1598,16 @@ def is_geostationary(self):
            return False
        return 'geostationary' in coord_operation.method_name.lower()

-    def _get_geo_boundary_sides(self, vertices_per_side=None):
-        """Retrieve the boundary sides list for geostationary projections."""
+    def _get_geostationary_boundary_sides(self, vertices_per_side=None, coordinates="geographic"):


I'm not a huge fan of this keyword argument, but this is also a private method at the moment so maybe it's OK for now. I can see how the beginning and end of the method are the same so splitting it into two separate methods based on coordinate type would be annoying. @mraspaud other ideas?

I would suggest to merge meanwhile, and we can split this in two functions later on ...
With this PR merged, I can open other 3 parallel PRs.

djhoese · 2023-12-08T17:20:48Z

pyresample/geometry.py

        new_dim1_sides = []
        new_dim2_sides = []
        for dim1_side, dim2_side in zip(dim1_sides, dim2_sides):
+            # FIXME: ~(~np.isfinite(dim1_side) | ~np.isfinite(dim1_side))


Does something need to happen with this?

Right now Inf that are present somewhere on the boundary can pass in the sides (and mess up stuffs in downstream tasks). But I can't set this now, because for all out-of-Earth boundary, removing every Inf and not retrieving boundary sides crash pykdtree and gradient.
I will fix this in the out-of-Earth boundary PR

djhoese · 2023-12-08T18:53:22Z

Ugh, that's what I get for trying to fix something in the github GUI. Working on it...

djhoese · 2023-12-08T19:04:02Z

I'll merge this when CI passes. Another idea (for a much later PR) I had and maybe mentioned during out last meeting:

How useful are some of these helper methods outside of boundary creation? What if the new Boundary classes had a .from_area or .from_geometry classmethod or how we talked about a factory function (something like boundary_for_geometry(...))? All these little helper methods could be moved there and out of the geometry.py module.

Add _geographic_sides and _projection_sides method

1a70237

ghiggi changed the title ~~Refactor area boundary sides retrieval with _geographic_sides and _projection_sides method~~ Refactor area boundary sides retrieval with _geographic_sides and _projection_sides method Dec 8, 2023

ghiggi changed the title ~~Refactor area boundary sides retrieval with _geographic_sides and _projection_sides method~~ Refactor area boundary sides retrieval with _geographic_sides and _projection_sides methods Dec 8, 2023

ghiggi added 3 commits December 8, 2023 15:09

Try fix conflicts

9a81880

Rerun tests

c1ec951

Refactor gradient polygon definition for swath

75b40bd

djhoese reviewed Dec 8, 2023

View reviewed changes

djhoese added the refactor label Dec 8, 2023

djhoese self-assigned this Dec 8, 2023

Add possible error types

856a4b2

djhoese reviewed Dec 8, 2023

View reviewed changes

ghiggi added 2 commits December 8, 2023 17:45

Merge branch 'main' into refactor-geometry-sides-creation

ace7d98

Merge main

c39c308

djhoese reviewed Dec 8, 2023

View reviewed changes

Try to fix type annotations

8805631

Fix type annotations in geometry.py

aee90a1

djhoese merged commit e82bf47 into pytroll:main Dec 8, 2023
22 of 27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor area boundary sides retrieval with `_geographic_sides` and `_projection_sides` methods #566

Refactor area boundary sides retrieval with `_geographic_sides` and `_projection_sides` methods #566

ghiggi commented Dec 8, 2023 •

edited

Loading

djhoese Dec 8, 2023

ghiggi Dec 8, 2023

djhoese Dec 8, 2023

codecov bot commented Dec 8, 2023 •

edited

Loading

coveralls commented Dec 8, 2023 •

edited

Loading

ghiggi commented Dec 8, 2023

djhoese Dec 8, 2023

ghiggi Dec 8, 2023

djhoese Dec 8, 2023

ghiggi Dec 8, 2023 •

edited

Loading

djhoese commented Dec 8, 2023

djhoese commented Dec 8, 2023 •

edited

Loading

Refactor area boundary sides retrieval with _geographic_sides and _projection_sides methods #566

Refactor area boundary sides retrieval with _geographic_sides and _projection_sides methods #566

Conversation

ghiggi commented Dec 8, 2023 • edited Loading

djhoese Dec 8, 2023

Choose a reason for hiding this comment

ghiggi Dec 8, 2023

Choose a reason for hiding this comment

djhoese Dec 8, 2023

Choose a reason for hiding this comment

codecov bot commented Dec 8, 2023 • edited Loading

Codecov Report

coveralls commented Dec 8, 2023 • edited Loading

ghiggi commented Dec 8, 2023

djhoese Dec 8, 2023

Choose a reason for hiding this comment

ghiggi Dec 8, 2023

Choose a reason for hiding this comment

djhoese Dec 8, 2023

Choose a reason for hiding this comment

ghiggi Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

djhoese commented Dec 8, 2023

djhoese commented Dec 8, 2023 • edited Loading

Refactor area boundary sides retrieval with `_geographic_sides` and `_projection_sides` methods #566

Refactor area boundary sides retrieval with `_geographic_sides` and `_projection_sides` methods #566

ghiggi commented Dec 8, 2023 •

edited

Loading

codecov bot commented Dec 8, 2023 •

edited

Loading

coveralls commented Dec 8, 2023 •

edited

Loading

ghiggi Dec 8, 2023 •

edited

Loading

djhoese commented Dec 8, 2023 •

edited

Loading