Add BoundarySides class
#568
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #568 +/- ##
==========================================
+ Coverage 94.09% 94.12% +0.03%
==========================================
Files 85 87 +2
Lines 13235 13312 +77
==========================================
+ Hits 12453 12530 +77
Misses 782 782
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@djhoese I think this is also ready to be merged. I will restart working on the other PR on Monday ;) |
|
|
||
| def __iter__(self): | ||
| """Return an iterator over the sides.""" | ||
| return iter(self._sides) |
There was a problem hiding this comment.
Do we need the iter() wrapper? I think you can just return self._sides.
There was a problem hiding this comment.
@djhoese I think iter is right for the purpose here, we are supposed to return an iterator, with self._sides isn't.
There was a problem hiding this comment.
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
| if not isinstance(index, int) or not 0 <= index < 4: | ||
| raise IndexError("Index must be an integer from 0 to 3.") |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I agree here, I think this function should just be return self._sides[index] and let python handle the rest.
| 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'] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
A dataclass would be an alternative
There was a problem hiding this comment.
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.
Co-authored-by: David Hoese <david.hoese@ssec.wisc.edu>
| @@ -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 | |||
There was a problem hiding this comment.
Why do you need to import this here?
| 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'] |
There was a problem hiding this comment.
A dataclass would be an alternative
| 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.") |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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 ...) ...
There was a problem hiding this comment.
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.
|
|
||
| def __iter__(self): | ||
| """Return an iterator over the sides.""" | ||
| return iter(self._sides) |
There was a problem hiding this comment.
@djhoese I think iter is right for the purpose here, we are supposed to return an iterator, with self._sides isn't.
| if not isinstance(index, int) or not 0 <= index < 4: | ||
| raise IndexError("Index must be an integer from 0 to 3.") |
There was a problem hiding this comment.
I agree here, I think this function should just be return self._sides[index] and let python handle the rest.
| 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'] |
There was a problem hiding this comment.
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.
git diff origin/main **/*py | flake8 --diffThis PR add the
BoundarySidesclass to facilitate the manipulation of boundary sides.