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

Add BoundarySides class #568

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions pyresample/boundary/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
"""The Boundary classes."""

from pyresample.boundary.boundary_sides import BoundarySides # noqa
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to import this 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.

I will remove it.

from pyresample.boundary.legacy_boundary import ( # noqa
AreaBoundary,
AreaDefBoundary,
Expand Down
86 changes: 86 additions & 0 deletions pyresample/boundary/boundary_sides.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
#!/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 BoundarySides class."""

import logging

import numpy as np

logger = logging.getLogger(__name__)


class BoundarySides:
"""A class to represent the sides of an area boundary.

The sides are stored as a tuple of 4 numpy arrays, each representing the
coordinate (geographic or projected) of the vertices of the boundary side.
The sides must be stored in the order (top, right, left, bottom),
which refers to the side position with respect to the coordinate array.
The first row of the coordinate array correspond to the top side, the last row to the bottom side,
the first column to the left side and the last column to the right side.
Please note that the last vertex of each side must be equal to the first vertex of the next side.
"""
__slots__ = ['_sides']
Comment on lines +27 to +38
Copy link
Member

Choose a reason for hiding this comment

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

I mentioned this in the large PR and I'm curious what @mraspaud thinks, but I think this entire class can be replaced with a namedtuple. A namedtuple would provide all the same functionality except the .vertices property but I think you can subclass the namedtuple to add that if you need. You could also then do your own docstrings as you have them.

Copy link
Member

Choose a reason for hiding this comment

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

A dataclass would be an alternative

Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that profiling done by some Python core devs have proven that data classes are not faster than any of the alternatives and in some cases are slower. I don't remember those cases and differences.

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 __slots__ needed 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.

I remember @djhoese suggesting me to use __slots__ whatever possible to speed up stuffs ...
Do we want to use a named_tuple instead @mraspaud? Or just a class without __slots__?

Copy link
Member

Choose a reason for hiding this comment

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

My thought on __slots__ is that low-level immutable classes should use it to reduce memory footprint and speedup execution. My opinion is that it should be used anywhere there isn't a strong reason not to (subclassing gets harder, etc). I still prefer a namedtuple here.


def __init__(self, sides: tuple[ArrayLike, ArrayLike, ArrayLike, ArrayLike]):
"""Initialize the BoundarySides object."""
if len(sides) != 4 or not all(isinstance(side, np.ndarray) and side.ndim == 1 for side in sides):
raise ValueError("Sides must be a list of four numpy arrays.")
Comment on lines +40 to +43
Copy link
Member

Choose a reason for hiding this comment

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

Instead of controlling the size of the argument, why not pass four arguments instead? this would make it also clear which one is top, bottom, etc...

Copy link
Contributor Author

@ghiggi ghiggi Dec 12, 2023

Choose a reason for hiding this comment

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

I defined like this because in geometry.py we always retrieve directly a list with the 4 elements.
Do you think it make sense to unpack @mraspaud?
Also consider that for some projections (the left and right) sides are just "dummy" and of size 2 (because we have a circle ...) ...

Copy link
Member

Choose a reason for hiding this comment

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

This is such an obvious solution it makes me angry that I didn't think of it in the giant pull request. I think the fact that left and right are sometimes "dummy" sides doesn't change anything unless there is some optimization by knowing that information, but that doesn't seem to be used here, right? ...I still think a namedtuple serves the same purpose.

I haven't used it yet, but I'm 90% sure that you can specify shape or dimensionality and dtype with the ArrayLike type annotation. That might add some more type-annotation-level information for the user and IDEs to make sure the right thing is passed.


if not all(np.array_equal(sides[i][-1], sides[(i + 1) % 4][0]) for i in range(4)):
raise ValueError("The last element of each side must be equal to the first element of the next side.")

self._sides = tuple(sides) # Store as a tuple

@property
def top(self):
"""Return the vertices of the top side."""
return self._sides[0]

@property
def right(self):
"""Return the vertices of the right side."""
return self._sides[1]

@property
def bottom(self):
"""Return the vertices of the bottom side."""
return self._sides[2]

@property
def left(self):
"""Return the vertices of the left side."""
return self._sides[3]

@property
def vertices(self):
"""Return the vertices of the concatenated sides.

Note that the last element of each side is discarded to avoid duplicates.
"""
return np.concatenate([side[:-1] for side in self._sides])

def __iter__(self):
"""Return an iterator over the sides."""
return iter(self._sides)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the iter() wrapper? I think you can just return self._sides.

Copy link
Member

Choose a reason for hiding this comment

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

@djhoese I think iter is right for the purpose here, we are supposed to return an iterator, with self._sides isn't.

Copy link
Member

Choose a reason for hiding this comment

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

self._sides is a tuple. A tuple is an Iterator isn't it? Ah ok, I guess it is an Iterable, but technically needs to support next(my_iter) to be an Iterator:

https://docs.python.org/3/library/collections.abc.html#collections-abstract-base-classes


def __getitem__(self, index):
"""Return the side at the given index."""
if not isinstance(index, int) or not 0 <= index < 4:
raise IndexError("Index must be an integer from 0 to 3.")
Comment on lines +84 to +85
Copy link
Member

Choose a reason for hiding this comment

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

I don't think provides much benefit over the error message that self._sides[index] gives by itself, right? You could also do a try/except IndexError if you really wanted to raise this. That way the index checks aren't happening multiple times (inside _sides[] and here).

Copy link
Member

Choose a reason for hiding this comment

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

I agree here, I think this function should just be return self._sides[index] and let python handle the rest.

return self._sides[index]
107 changes: 107 additions & 0 deletions pyresample/test/test_boundary/test_boundary_sides.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
#!/usr/bin/env python
# -*- coding: utf-8 -*-
# pyresample, Resampling of remote sensing image data in python
#
# Copyright (C) 2010-2022 Pyresample developers
#
# This program is free software: you can redistribute it and/or modify it under
# the terms of the GNU Lesser 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 Lesser General Public License for more
# details.
#
# You should have received a copy of the GNU Lesser General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
"""Test the BoundarySides objects."""

import numpy as np
import pytest

from pyresample.boundary import BoundarySides


class TestBoundarySides:
"""Test suite for the BoundarySides class with 1D numpy arrays for sides."""

def test_initialization_valid_input(self):
"""Test initialization with valid 1D numpy array inputs."""
sides = [np.array([1, 2, 3]), # top
np.array([3, 4, 5]), # right
np.array([5, 6, 7]), # bottom
np.array([7, 8, 1])] # left
boundary = BoundarySides(sides)
assert all(np.array_equal(boundary[i], sides[i]) for i in range(4))

def test_initialization_invalid_input(self):
"""Test initialization with invalid inputs, such as wrong number of sides or non-1D arrays."""
with pytest.raises(ValueError):
BoundarySides([np.array([1, 2]), # Invalid number of sides
np.array([2, 3])])

with pytest.raises(ValueError):
BoundarySides([np.array([1, 2]), # Non-1D arrays
np.array([[2, 3], [4, 5]]),
np.array([5, 6]),
np.array([6, 7])])

with pytest.raises(ValueError):
BoundarySides([np.array([1, 2]), # Invalid side connection
np.array([3, 4]),
np.array([4, 6]),
np.array([6, 1])])

def test_property_accessors(self):
"""Test property accessors with 1D numpy arrays."""
sides = [np.array([1, 2, 3]), # top
np.array([3, 4, 5]), # right
np.array([5, 6, 7]), # bottom
np.array([7, 8, 1])] # left
boundary = BoundarySides(sides)
assert np.array_equal(boundary.top, sides[0])
assert np.array_equal(boundary.right, sides[1])
assert np.array_equal(boundary.bottom, sides[2])
assert np.array_equal(boundary.left, sides[3])

def test_vertices_property(self):
"""Test the vertices property with concatenated 1D numpy arrays."""
sides = [np.array([1, 2, 3]), # top
np.array([3, 4, 5]), # right
np.array([5, 6, 7]), # bottom
np.array([7, 8, 1])] # left
boundary = BoundarySides(sides)
expected_vertices = np.array([1, 2, 3, 4, 5, 6, 7, 8])
assert np.array_equal(boundary.vertices, expected_vertices)

def test_iteration(self):
"""Test iteration over the 1D numpy array sides."""
sides = [np.array([1, 2, 3]), # top
np.array([3, 4, 5]), # right
np.array([5, 6, 7]), # bottom
np.array([7, 8, 1])] # left
boundary = BoundarySides(sides)
for i, side in enumerate(boundary):
assert np.array_equal(side, sides[i])

def test_indexing_valid(self):
"""Test valid indexing with 1D numpy arrays."""
sides = [np.array([1, 2, 3]), # top
np.array([3, 4, 5]), # right
np.array([5, 6, 7]), # bottom
np.array([7, 8, 1])] # left
boundary = BoundarySides(sides)
for i in range(4):
assert np.array_equal(boundary[i], sides[i])

def test_indexing_invalid(self):
"""Test indexing with invalid indices."""
sides = [np.array([1, 2, 3]), # top
np.array([3, 4, 5]), # right
np.array([5, 6, 7]), # bottom
np.array([7, 8, 1])] # left
boundary = BoundarySides(sides)
with pytest.raises(IndexError):
boundary[4] # Invalid index
Loading