Add `BoundarySides` class #568

ghiggi · 2023-12-08T19:08:52Z

This PR add the BoundarySides class to facilitate the manipulation of boundary sides.

codecov · 2023-12-08T19:15:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c240c04) 94.09% compared to head (47e2004) 94.12%.
Report is 10 commits behind head on main.

❗ Current head 47e2004 differs from pull request most recent head a2e9fb1. Consider uploading reports for the commit a2e9fb1 to get more accurate results

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

Flag	Coverage Δ
unittests	`94.12% <100.00%> (+0.03%)`	⬆️

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.

ghiggi · 2023-12-08T19:15:57Z

@djhoese I think this is also ready to be merged. I will restart working on the other PR on Monday ;)
Have a nice week-end

coveralls · 2023-12-08T19:34:11Z

coverage: 93.711% (+0.04%) from 93.675%
when pulling 47e2004 on ghiggi:add-boundary-sides-class
into c240c04 on pytroll:main.

pyresample/boundary/boundary_sides.py

djhoese · 2023-12-08T19:58:50Z

pyresample/boundary/boundary_sides.py

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


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

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

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

djhoese · 2023-12-08T20:00:06Z

pyresample/boundary/boundary_sides.py

+        if not isinstance(index, int) or not 0 <= index < 4:
+            raise IndexError("Index must be an integer from 0 to 3.")


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).

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

djhoese · 2023-12-08T20:01:25Z

pyresample/boundary/boundary_sides.py

+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']


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.

A dataclass would be an alternative

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 <[email protected]>

mraspaud

Nice work, however, this is not used anywhere? Is there a lot of code to change to use this instead of the current sides in the rest of pyresample?

mraspaud · 2023-12-12T10:40:12Z

pyresample/boundary/__init__.py

@@ -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


Why do you need to import this here?

I will remove it.

mraspaud · 2023-12-12T10:41:20Z

pyresample/boundary/boundary_sides.py

+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']


A dataclass would be an alternative

mraspaud · 2023-12-12T10:43:03Z

pyresample/boundary/boundary_sides.py

+    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.")


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

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 ...) ...

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.

mraspaud · 2023-12-12T10:44:25Z

pyresample/boundary/boundary_sides.py

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


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

mraspaud · 2023-12-12T10:45:31Z

pyresample/boundary/boundary_sides.py

+        if not isinstance(index, int) or not 0 <= index < 4:
+            raise IndexError("Index must be an integer from 0 to 3.")


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

mraspaud · 2023-12-12T10:46:11Z

pyresample/boundary/boundary_sides.py

+    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']


Why is __slots__ needed here?

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__?

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.

ghiggi added 2 commits December 8, 2023 20:06

Add BoundarySides class

3563b7a

Fix precommit issues

47e2004

djhoese requested changes Dec 8, 2023

View reviewed changes

Update pyresample/boundary/boundary_sides.py

a2e9fb1

Co-authored-by: David Hoese <[email protected]>

mraspaud requested changes Dec 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `BoundarySides` class #568

Add `BoundarySides` class #568

ghiggi commented Dec 8, 2023 •

edited

Loading

codecov bot commented Dec 8, 2023 •

edited

Loading

ghiggi commented Dec 8, 2023

coveralls commented Dec 8, 2023

djhoese Dec 8, 2023

mraspaud Dec 12, 2023

djhoese Dec 13, 2023

djhoese Dec 8, 2023

mraspaud Dec 12, 2023

djhoese Dec 8, 2023

mraspaud Dec 12, 2023

djhoese Dec 13, 2023

mraspaud left a comment

mraspaud Dec 12, 2023

ghiggi Dec 12, 2023

mraspaud Dec 12, 2023

mraspaud Dec 12, 2023

ghiggi Dec 12, 2023 •

edited

Loading

djhoese Dec 13, 2023

mraspaud Dec 12, 2023

mraspaud Dec 12, 2023

mraspaud Dec 12, 2023

ghiggi Dec 12, 2023

djhoese Dec 13, 2023

		if not isinstance(index, int) or not 0 <= index < 4:
		raise IndexError("Index must be an integer from 0 to 3.")

Add BoundarySides class #568

Are you sure you want to change the base?

Add BoundarySides class #568

Conversation

ghiggi commented Dec 8, 2023 • edited Loading

codecov bot commented Dec 8, 2023 • edited Loading

Codecov Report

ghiggi commented Dec 8, 2023

coveralls commented Dec 8, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

ghiggi Dec 12, 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

Choose a reason for hiding this comment

Add `BoundarySides` class #568

Add `BoundarySides` class #568

ghiggi commented Dec 8, 2023 •

edited

Loading

codecov bot commented Dec 8, 2023 •

edited

Loading

ghiggi Dec 12, 2023 •

edited

Loading