Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor for boundary lat/lon extraction #546

Open
wants to merge 40 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 39 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
e203899
Add refactor
ghiggi Oct 10, 2023
95decd3
Fix wrong import
ghiggi Oct 10, 2023
ae730f7
Lint
ghiggi Oct 10, 2023
edcfcb4
Fix typo
ghiggi Oct 11, 2023
eae1873
Fix issues
ghiggi Oct 11, 2023
1662d2d
Fix issues
ghiggi Oct 11, 2023
655fb72
Fix issues
ghiggi Oct 11, 2023
b7df9be
Improve doc
ghiggi Oct 11, 2023
a5322a6
Improve doc
ghiggi Oct 11, 2023
88065d7
Remove old use of frequency argument
ghiggi Oct 11, 2023
0e992a2
Merge branch 'main' into refactor-get_bbox-lonlat
djhoese Nov 22, 2023
8eff9b4
Address refactor boundary sides
ghiggi Nov 22, 2023
36795df
Fix tests related to gradient resampling
ghiggi Nov 22, 2023
1dca712
Deprecate get_edge_lonlat and remove duplicated coordinates
ghiggi Nov 22, 2023
3c60e74
Ensure shapely polygon is closed !
ghiggi Nov 22, 2023
bba3bde
Refactor get_polygon and get_border_lonlat in gradient
ghiggi Nov 22, 2023
34edc5c
Initial refactor of AreaBoundary
ghiggi Nov 22, 2023
8fbf02f
Deprecate get_bbox_lonlats
ghiggi Nov 23, 2023
d9f4165
Private get_polygon and get_border_lonlat in gradient.__init__
ghiggi Nov 23, 2023
83ec6df
Add test area fixtures
ghiggi Nov 23, 2023
9de0a44
Treat geo area inside earth disk as classical AreaDef
ghiggi Nov 23, 2023
efd0773
Deprecate get_edge_bbox_in_projection_coordinates
ghiggi Nov 23, 2023
0b669f8
Refactor boundary classes and ensure backward compatibilities
ghiggi Nov 23, 2023
cea363f
Add BoundarySides test units
ghiggi Nov 23, 2023
b9cda35
Add test units for visualizion utilities
ghiggi Nov 23, 2023
d41973a
Consistent naming across repo of lon/lat sides with sides_lons and si…
ghiggi Nov 23, 2023
c6a9378
Add geographic_boundary and projection_boundary methods
ghiggi Nov 23, 2023
6323814
Deprecate boundary() for geographic_boundary()
ghiggi Nov 24, 2023
6210bea
Pass AreaDef crs to ProjectionBoundary crs
ghiggi Nov 24, 2023
28b5527
Solve wasted polygon computations in gradient for GEO FD
ghiggi Nov 24, 2023
0eb7a6b
Deprecate get_boundary_lonlats and SimpleBoundary
ghiggi Nov 24, 2023
0f1b578
Remove use of __file__ and relative paths in test units
ghiggi Nov 24, 2023
059d496
Improve clarity of sides extraction for geographic and projection coo…
ghiggi Nov 24, 2023
d35f1b5
Refactor Geographic and Projection Boundary using composition and dep…
ghiggi Nov 24, 2023
b3273b8
Deal with AreaDefinition with unvalid sides: polar projections, globa…
ghiggi Nov 25, 2023
e3f4b24
Add code structure for boundary extraction for polar and global plana…
ghiggi Nov 25, 2023
915e34f
Fix boundary ordering logic for swath and projections !
ghiggi Nov 27, 2023
61240eb
Fix formatting warnings
ghiggi Nov 27, 2023
7069197
Renaming to SphericalBoundary and PlanarBoundary
ghiggi Nov 27, 2023
e638557
Some cleanout and notes
ghiggi Nov 27, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions docs/source/howtos/spherical_geometry.rst
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,13 @@ satellite passes. See trollschedule_ how to generate a list of satellite overpas
`area_def` is an :class:`~pyresample.geometry.AreaDefinition` object.

>>> from pyresample.spherical_utils import GetNonOverlapUnions
>>> from pyresample.boundary import AreaDefBoundary

>>> area_boundary = AreaDefBoundary(area_def, frequency=100) # doctest: +SKIP
>>> area_boundary = area_boundary.contour_poly # doctest: +SKIP
>>> area_boundary = area_def.boundary(vertices_per_side=100) # doctest: +SKIP
>>> area_boundary = area_boundary.polygon # doctest: +SKIP

>>> list_of_polygons = []
>>> for mypass in passes: # doctest: +SKIP
>>> list_of_polygons.append(mypass.boundary.contour_poly) # doctest: +SKIP
>>> list_of_polygons.append(mypass.boundary().polygon) # doctest: +SKIP

>>> non_overlaps = GetNonOverlapUnions(list_of_polygons) # doctest: +SKIP
>>> non_overlaps.merge() # doctest: +SKIP
Expand Down
9 changes: 7 additions & 2 deletions pyresample/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,13 @@

from .version import get_versions # noqa

__all__ = ['grid', 'image', 'kd_tree', 'utils', 'plot', 'geo_filter', 'geometry', 'CHUNK_SIZE',
'load_area', 'create_area_def', 'get_area_def', 'parse_area_file', 'convert_def_to_yaml']
_root_path = os.path.dirname(os.path.realpath(__file__))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from pathlib import Path
pkg_root_path = Path(__file__).resolve().parent

And is this used outside of the test modules? If not, I'd prefer this go in pyresample/test/__init__.py or maybe in conftest.py but I think it'd have to be a fixture to be useful there and that probably isn't worth it. You'd have to do the right amount of .parent.parent to get the correct directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok


__all__ = [
'grid', 'image', 'kd_tree', 'utils', 'plot', 'geo_filter', 'geometry', 'CHUNK_SIZE',
'load_area', 'create_area_def', 'get_area_def', 'parse_area_file', 'convert_def_to_yaml',
"_root_path",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this listed in __all__. The all variable is meant to deal with from pyresample import *. No need for _root_path to be there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. I didn't know the purpose of __all__ was for that :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On this topic, from pyresample import *is not recommended in python this a while, should we just remove __all__?

]

__version__ = get_versions()['version']
del get_versions
1 change: 1 addition & 0 deletions pyresample/_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"features": {
"future_geometries": False,
},
"force_boundary_computations": False,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep this it will need documentation, but that can be the last thing in this PR.

}],
paths=_CONFIG_PATHS,
)
16 changes: 8 additions & 8 deletions pyresample/bilinear/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -599,14 +599,14 @@ def get_valid_indices_from_lonlat_boundaries(
target_geo_def, source_lons, source_lats, radius_of_influence):
"""Get valid indices from lonlat boundaries."""
# Resampling from swath to grid or from grid to grid
lonlat_boundary = target_geo_def.get_boundary_lonlats()

# Combine reduced and legal values
return data_reduce.get_valid_index_from_lonlat_boundaries(
lonlat_boundary[0],
lonlat_boundary[1],
source_lons, source_lats,
radius_of_influence)
try:
sides_lons, sides_lats = target_geo_def.boundary().sides
valid_indices = data_reduce.get_valid_index_from_lonlat_boundaries(sides_lons, sides_lats,
source_lons, source_lats,
radius_of_influence)
except Exception:
valid_indices = np.ones(source_lons.size, dtype=bool)
Comment on lines +607 to +608
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this was done to make tests pass? What's going on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No @djhoese. This try-except is used to deal with out of Earth disk projections !!!
Before this PR, the boundary was created using get_boundary_lonlats (see

def get_boundary_lonlats(self):
). The method was just naively taking the image border lon/latcoordinates.
So for the geostationary full disk and other projections with out-of-earth disk at the border, the boundary sides were all Inf. And inside the .get_valid_index_from_lonlat_boundaries if there was any invalid side coordinate, it was returning an array of ones (no data reduction done !). This means that for full disc GEO projections we were not doing data reduction !!!

In this PR, I replaced get_boundary_lonlats with target_geo_def.boundary().sides. So:

  • if the area is geostationary, now we finally reduce data (and this should speed up stuffs !!!)
  • if the area boundary has out-of-Earth locations and the flag force_boundary_computations is True, the boundary method will retrieve the actual boundary coordinate inside the area, and we will reduce data ...
  • if the area boundary has out-of-Earth locations and the flag force_boundary_computations is False (to keep the old behaviour), the boundary method now fails because if all sides are Inf it will raise a ValueError within _filter_bbox_nans. If this happen, we return what we returned before: the np.ones array

return valid_indices


def get_slicer(data):
Expand Down
32 changes: 32 additions & 0 deletions pyresample/boundary/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#!/usr/bin/env python
# -*- coding: utf-8 -*-
#
# Copyright (c) 2014-2021 Pyresample developers
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
"""The Boundary classes."""

from pyresample.boundary.area_boundary import AreaBoundary, AreaDefBoundary
from pyresample.boundary.planar_boundary import PlanarBoundary
from pyresample.boundary.simple_boundary import SimpleBoundary
from pyresample.boundary.spherical_boundary import SphericalBoundary

__all__ = [
"SphericalBoundary",
"PlanarBoundary",
# Deprecated
"SimpleBoundary",
"AreaBoundary",
"AreaDefBoundary",
]
Comment on lines +25 to +32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove __all__. It is considered bad practice to do from module import * in python, so we should not encourage it by providing __all__.

32 changes: 11 additions & 21 deletions pyresample/boundary.py → pyresample/boundary/area_boundary.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#!/usr/bin/env python
# -*- coding: utf-8 -*-
#
# Copyright (c) 2014-2021 Pyresample developers
# Copyright (c) 2014-2023 Pyresample developers
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
Expand All @@ -15,8 +15,7 @@
#
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
"""The Boundary classes."""

"""Deprecated Boundary, AreaBoundary, AreaDefBoundary class."""
import logging
import warnings

Expand Down Expand Up @@ -44,6 +43,9 @@ def contour(self):
@property
def contour_poly(self):
"""Get the Spherical polygon corresponding to the Boundary."""
warnings.warn("'contour_poly' is deprecated." +
"Use the 'boundary().polygon' property instead!.",
PendingDeprecationWarning, stacklevel=2)
if self._contour_poly is None:
self._contour_poly = SphPolygon(
np.deg2rad(np.vstack(self.contour()).T))
Expand All @@ -62,6 +64,9 @@ class AreaBoundary(Boundary):

def __init__(self, *sides):
Boundary.__init__(self)
warnings.warn("'AreaBoundary' will be removed in the future. " +
"Use the Swath/AreaDefinition 'boundary' method instead!.",
PendingDeprecationWarning, stacklevel=2)
# Check 4 sides are provided
if len(sides) != 4:
raise ValueError("AreaBoundary expects 4 sides.")
Expand Down Expand Up @@ -117,28 +122,13 @@ def decimate(self, ratio):


class AreaDefBoundary(AreaBoundary):
"""Boundaries for area definitions (pyresample)."""
"""Boundaries for a pyresample AreaDefinition."""

def __init__(self, area, frequency=1):
lons, lats = area.get_bbox_lonlats()
sides_lons, sides_lats = area.boundary().sides
warnings.warn("'AreaDefBoundary' will be removed in the future. " +
"Use the Swath/AreaDefinition 'boundary' method instead!.",
PendingDeprecationWarning, stacklevel=2)
AreaBoundary.__init__(self,
*zip(lons, lats))

AreaBoundary.__init__(self, *zip(sides_lons, sides_lats))
if frequency != 1:
self.decimate(frequency)


class SimpleBoundary(object):
"""Container for geometry boundary.

Labelling starts in upper left corner and proceeds clockwise
"""

def __init__(self, side1, side2, side3, side4):
self.side1 = side1
self.side2 = side2
self.side3 = side3
self.side4 = side4
127 changes: 127 additions & 0 deletions pyresample/boundary/base_boundary.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
#!/usr/bin/env python
# -*- coding: utf-8 -*-
#
# Copyright (c) 2014-2023 Pyresample developers
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
"""Define the BaseBoundary class."""

import logging

import numpy as np

from pyresample.boundary.sides import BoundarySides

logger = logging.getLogger(__name__)


class BaseBoundary:
"""Base class for boundary objects."""
__slots__ = ["_sides_x", "_sides_y"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you motivate the use of __slots__ here? I don't see any real benefit in this class.


def __init__(self, area, vertices_per_side=None):

sides_x, sides_y = self._compute_boundary_sides(area, vertices_per_side)
self._sides_x = BoundarySides(sides_x)
self._sides_y = BoundarySides(sides_y)
self._area = area

self.is_clockwise = self._check_is_boundary_clockwise(sides_x, sides_y, area)
self.is_counterclockwise = not self.is_clockwise
self._set_order(order=None) # FIX !

def _check_is_boundary_clockwise(self, sides_x, sides_y, area):
"""Check if the boundary is clockwise or counterclockwise."""
raise NotImplementedError()

def _compute_boundary_sides(self, area, vertices_per_side):
"""Compute boundary sides."""
raise NotImplementedError()
Comment on lines +48 to +50
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the base class should be abstract, right?


def _set_order(self, order):
"""Set the order of the boundary vertices."""
if self.is_clockwise:
self._actual_order = "clockwise"
else:
self._actual_order = "counterclockwise"

if order is None:
self._wished_order = self._actual_order
else:
if order not in ["clockwise", "counterclockwise"]:
raise ValueError("Valid 'order' is 'clockwise' or 'counterclockwise'")
self._wished_order = order

def set_clockwise(self):
"""Set clockwise order for vertices retrieval."""
self._wished_order = "clockwise"
return self

def set_counterclockwise(self):
"""Set counterclockwise order for vertices retrieval."""
self._wished_order = "counterclockwise"
return self

@property
def sides(self):
"""Return the boundary sides as a tuple of (sides_x, sides_y) arrays."""
return self._sides_x, self._sides_y

@property
def _x(self):
"""Retrieve boundary x vertices."""
xs = self._sides_x.vertices
if self._wished_order == self._actual_order:
return xs
else:
return xs[::-1]

@property
def _y(self):
"""Retrieve boundary y vertices."""
ys = self._sides_y.vertices
if self._wished_order == self._actual_order:
return ys
else:
return ys[::-1]

@property
def vertices(self):
"""Return boundary vertices 2D array [x, y]."""
vertices = np.vstack((self._x, self._y)).T
vertices = vertices.astype(np.float64, copy=False) # Important for spherical ops.
return vertices

def contour(self, closed=False):
"""Return the (x, y) tuple of the boundary object.

If excludes the last element of each side because it's included in the next side.
If closed=False (the default), the last vertex is not equal to the first vertex
If closed=True, the last vertex is set to be equal to the first
closed=True is required for shapely Polygon creation.
closed=False is required for pyresample SPolygon creation.
"""
x = self._x
y = self._y
if closed:
x = np.hstack((x, x[0]))
y = np.hstack((y, y[0]))
return x, y

def to_shapely_polygon(self):
"""Define a Shapely Polygon."""
from shapely.geometry import Polygon
self = self.set_counterclockwise()
x, y = self.contour(closed=True)
return Polygon(zip(x, y))
94 changes: 94 additions & 0 deletions pyresample/boundary/planar_boundary.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
#!/usr/bin/env python
# -*- coding: utf-8 -*-
#
# Copyright (c) 2014-2023 Pyresample developers
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
"""Define the PlanarBoundary class."""

import logging

import numpy as np

from pyresample.boundary.base_boundary import BaseBoundary

logger = logging.getLogger(__name__)


def _is_projection_boundary_clockwise(sides_x, sides_y):
"""Determine if the boundary is clockwise-defined in planar coordinates."""
from shapely.geometry import Polygon
x = np.concatenate([xs[:-1] for xs in sides_x])
y = np.concatenate([ys[:-1] for ys in sides_y])
x = np.hstack((x, x[0]))
y = np.hstack((y, y[0]))
polygon = Polygon(zip(x, y))
return not polygon.exterior.is_ccw


class PlanarBoundary(BaseBoundary):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is not sufficiently tested.

"""Projection Boundary object.

The inputs must be the x and y sides of the projection.
It expects the projection coordinates to be planar (i.e. metric, radians).
"""

@classmethod
def _check_is_boundary_clockwise(cls, sides_x, sides_y, area=None):
"""SphericalBoundary specific implementation."""
return _is_projection_boundary_clockwise(sides_x=sides_x, sides_y=sides_y)
Comment on lines +47 to +50
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this version a classmethod?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method must be called within BaseBoundary when we initiate the PlanarBoundary class.
But maybe it's not necessary to be a class method? I am not a class expert 😃 ... I will try to remove it and see what's going on ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

staticmethod and remove the cls/self argument would be the usual way of doing this...or at least what I expected to see.


@classmethod
def _compute_boundary_sides(cls, area, vertices_per_side):
sides_x, sides_y = area._get_projection_sides(vertices_per_side=vertices_per_side)
return sides_x, sides_y
Comment on lines +52 to +55
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be a static method.


def __init__(self, area, vertices_per_side=None):
super().__init__(area=area, vertices_per_side=vertices_per_side)

self.sides_x = self._sides_x
self.sides_y = self._sides_y
self.crs = self._area.crs
self.cartopy_crs = self._area.to_cartopy_crs()
Comment on lines +57 to +63
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be at the top of the class


@property
def _point_inside(self):
x, y = self._area.get_proj_coords(data_slice=(int(self.shape[0] / 2), int(self.shape[1] / 2)))
return x.squeeze(), y.squeeze()

@property
def x(self):
"""Retrieve boundary x vertices."""
return self._x

@property
def y(self):
"""Retrieve boundary y vertices."""
return self._y

def plot(self, ax=None, subplot_kw=None, crs=None, alpha=0.6, **kwargs):
"""Plot the the boundary. crs must be a Cartopy CRS !"""
from pyresample.visualization.geometries import plot_geometries

if self.cartopy_crs is None and crs is None:
raise ValueError("Projection Cartopy 'crs' is required to display projection boundary.")
if crs is None:
crs = self.cartopy_crs
if subplot_kw is None:
subplot_kw = {"projection": crs}

geom = self.to_shapely_polygon()
p = plot_geometries(geometries=[geom], crs=crs,
ax=ax, subplot_kw=subplot_kw, alpha=alpha, **kwargs)
return p
Loading
Loading