-
Notifications
You must be signed in to change notification settings - Fork 8
Docstrings and type hints #12
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
base: master
Are you sure you want to change the base?
Changes from all commits
49d8890
a99fd0a
1f20e04
c2a54f4
46febc3
9db83f8
d273e36
9336180
7b2d80d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,9 @@ | ||
| """ | ||
| Utility module for the calculation of alpha shapes | ||
| This is a core module of package which contains exceptions, functions | ||
| and classes essential to creating and working with figures. | ||
| """ | ||
| from typing import Tuple | ||
|
|
||
| from typing import Tuple, Union | ||
|
|
||
| import numpy as np | ||
| from matplotlib.tri import Triangulation | ||
|
|
@@ -11,28 +13,42 @@ | |
|
|
||
|
|
||
| class AlphaException(Exception): | ||
| """Abstract class for exceptions which could be raised during the work of Alpha_Shaper class.""" | ||
| pass | ||
|
|
||
|
|
||
| class NotEnoughPoints(AlphaException): | ||
| """Raised when an operation requires a certain number of points and that condition is not met.""" | ||
| pass | ||
|
|
||
|
|
||
| class OptimizationFailure(AlphaException): | ||
| """Raised when the conditions for optimization are not met.""" | ||
| pass | ||
|
|
||
|
|
||
| class OptimizationWarnging(UserWarning): | ||
| class OptimizationWarning(UserWarning): | ||
| """Warns user without interrupting the program.""" | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is more like a personal note of how warnings work. Also it is not very accurate, since you can always change the way warnings are treated (e.g. ignore them or treat them as errors). I would remove this docstring. |
||
| pass | ||
|
|
||
|
|
||
| class Delaunay(Triangulation): | ||
| """ | ||
| """Abstract class with useful interface. | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid using the term "abstract class", unless you are using abstract base classes and feel that you need to communicate this to the user |
||
| Visitor sublclass of matplotlib.tri.Triangulation. | ||
| Mimics scipy.spatial.Delaunay interface. | ||
| See similar idea on scipy.spatial.Delaunay solution. | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually what I meant is that this class implements (part of) the interface of scipy.spatial.Delaunay. I do not remember why I made this choice. Consider replacing this line with the previous sentence (or an improved version of it) |
||
| """ | ||
|
|
||
| def __init__(self, coords: NDArray): | ||
| def __init__(self, coords: NDArray) -> None: | ||
| """Set the interface object and pass the coords into it. | ||
|
|
||
| Args: | ||
| coords(NDArray): raw coords at which preparation process will be performed. | ||
|
|
||
| Raises: | ||
| - NotEnoughPoints: If there are fewer than 3 points provided. | ||
| - ValueError: For other value-related issues with the coordinates. | ||
| """ | ||
|
Comment on lines
+44
to
+50
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice. Try hinting the user about the expected shape of the |
||
|
|
||
| try: | ||
| super().__init__(x=coords[:, 0], y=coords[:, 1]) | ||
| except ValueError as e: | ||
|
|
@@ -42,17 +58,31 @@ def __init__(self, coords: NDArray): | |
| raise | ||
|
|
||
| @property | ||
| def simplices(self): | ||
| def simplices(self) -> NDArray: | ||
| """Return the collection of triangles.""" | ||
| return self.triangles | ||
|
|
||
| def __len__(self): | ||
| def __len__(self) -> int: | ||
| """Return amount of object's simplices.""" | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, but I'd prefer using 'number' instead of 'amount' |
||
| return self.simplices.shape[0] | ||
|
|
||
|
|
||
| class Alpha_Shaper(Delaunay): | ||
| """The class enables the creation of shapes and further operations on them.""" | ||
|
|
||
| mask: NDArray # for type hinting | ||
|
panosz marked this conversation as resolved.
|
||
|
|
||
| def __init__(self, points: ArrayLike, normalize=True): | ||
| def __init__(self, points: ArrayLike, normalize=True) -> None: | ||
| """Pass points into shaper. Optionally perform normalization. | ||
|
|
||
| Args: | ||
| points(ArrayLike): points used to create shapes. | ||
|
|
||
| normalize(bool): The flag determines whether the normalization process will be carried out. | ||
| See more about normalization on https://github.com/panosz/alpha_shapes. | ||
|
|
||
| """ | ||
|
|
||
| self.normalized = normalize | ||
|
|
||
| points = np.array(points) | ||
|
|
@@ -65,48 +95,67 @@ def __init__(self, points: ArrayLike, normalize=True): | |
| else: | ||
| self._initialize(points) | ||
|
|
||
| def _initialize(self, points: NDArray): | ||
| """ | ||
| _initialize the alpha shaper. | ||
| def _initialize(self, points: NDArray) -> None: | ||
| """_initialize the alpha shaper. | ||
|
|
||
| Args: | ||
| points(NDArray): points at which normalization will be performed. | ||
|
|
||
| """ | ||
|
|
||
| super().__init__(points) | ||
|
|
||
| self.circumradii_sq = self._calculate_cirumradii_sq_of_internal_triangles() | ||
| self.argsort = np.argsort(self.circumradii_sq) | ||
| default_mask = np.full_like(self.circumradii_sq, False, dtype=bool) | ||
| self.set_mask(default_mask) | ||
|
|
||
| def _denormalize(self, center, scale): | ||
| def _denormalize(self, center: NDArray, scale: NDArray) -> None: | ||
| """Transform back points into their original scale.""" | ||
| self.x = self.x * scale[0] + center[0] | ||
| self.y = self.y * scale[1] + center[1] | ||
|
|
||
| def _calculate_cirumradii_sq_of_internal_triangles(self): | ||
| def _calculate_cirumradii_sq_of_internal_triangles(self) -> NDArray: | ||
| """Method calculates circumradiuses squares of all internal triangles.""" | ||
|
|
||
| circumradii_sq = [ | ||
| self._get_circumradius_sq_of_internal_simplex(smpl) | ||
| for smpl in self.simplices | ||
| ] | ||
| return np.array(circumradii_sq) | ||
|
|
||
| def _get_circumradius_sq_of_internal_simplex(self, smpl): | ||
| def _get_circumradius_sq_of_internal_simplex(self, smpl: slice) -> NDArray: | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here smpl is a numpy array of integers, not a slice. It could work with a slice as well, but, since this is an "internal" function we know exactly how it is being used. In general, please be careful when adding type hints to existing functions. Use breakpoints to verify that the types are what you think they are. I suggest you take a second look at all the type hints you have added, even those in your previous commits. There may be a few errors that I missed in the last review. One that I have spotted is the type hint for the return value of |
||
| """Read value of squared circumradius of internal triangle.""" | ||
| x = self.x[smpl] | ||
| y = self.y[smpl] | ||
| return _calculate_cirumradius_sq_of_triangle(x, y) | ||
|
|
||
| def _sorted_simplices(self): | ||
| def _sorted_simplices(self) -> NDArray[np.float64]: | ||
| """Return the collection of simplices, sorted by their circumradius.""" | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding info about the shape of the returned value in the docstring |
||
| return self.simplices[self.argsort] | ||
|
|
||
| def _sorted_circumradii_sw(self) -> NDArray[np.float64]: | ||
| """Return sorted values of squares circumradiuses of internal triangles.""" | ||
| return self.circumradii_sq[self.argsort] | ||
|
|
||
| def _shape_from_simplices(self, simplices): | ||
| def _shape_from_simplices(self, simplices: ArrayLike) -> ArrayLike: | ||
| """Return the shape from simplices. | ||
| Output is in unary_union form which makes further operations on a shape easier. """ | ||
|
|
||
| triangles = [_simplex_to_triangle(smpl, self) for smpl in simplices] | ||
|
|
||
| return unary_union(triangles) | ||
|
|
||
| def get_mask(self, alpha): | ||
| def get_mask(self, alpha: float) -> NDArray: | ||
| """Return mask, based on squares of circumradiuses of internal triangles. | ||
| Mask specifies which elements should be considered for triangulation.""" | ||
| return self.circumradii_sq > 1 / alpha**2 | ||
|
|
||
| def get_shape(self, alpha): | ||
| def get_shape(self, alpha: float) -> ArrayLike: | ||
| """Return shape, based on the given alpha. | ||
| If alpha is less or equal to 0, the shape creation will be based on external points. | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit confusing. I guess what you want to say is that for alpha<=0 the shape is the convex hull of all the points |
||
| """ | ||
|
|
||
| if alpha > 0: | ||
| select = self.circumradii_sq <= 1 / alpha**2 | ||
| simplices = self.simplices[select] | ||
|
|
@@ -115,29 +164,30 @@ def get_shape(self, alpha): | |
|
|
||
| return self._shape_from_simplices(simplices) | ||
|
|
||
| def _nth_shape(self, n): | ||
| """ | ||
| return the shape formed by the n smallest simplices | ||
| """ | ||
| def _nth_shape(self, n: int) -> ArrayLike: | ||
| """Return the shape formed by the amount of n-smallest simplices.""" | ||
| simplices = self._sorted_simplices()[:n] | ||
| return self._shape_from_simplices(simplices) | ||
|
|
||
| def all_vertices(self): | ||
| def all_vertices(self) -> set: | ||
| """Return all vertices of object.""" | ||
| return set(np.ravel(self.simplices)) | ||
|
|
||
| def _uncovered_vertices(self, simplices): | ||
| """ | ||
| Return a set of vertices that is not covered by the | ||
| specified simplices. | ||
| """ | ||
| def _uncovered_vertices(self, simplices: ArrayLike) -> set: | ||
| """Return a set of vertices, which is not covered by the specified simplices.""" | ||
| return self.all_vertices() - set(np.ravel(simplices)) | ||
|
|
||
| def _get_minimum_fully_covering_index_of_simplices(self) -> int: | ||
| """ | ||
| Return the minimum index of simplices needed to cover all vertices. | ||
| """Return the minimum index of simplices needed to cover all vertices. | ||
| The set of all simplices up to this index is fully covering. | ||
|
|
||
| Raises: | ||
| - OptimizationFailure: For issues when the conditions for optimization are not met. | ||
| A common issue is duplicate points in the dataset. | ||
|
|
||
|
Comment on lines
+183
to
+187
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is nice. I like it how you give a simple example of when the exception may be raised. |
||
| """ | ||
| # At least N//3 triangles are needed to connect N points. | ||
|
|
||
| # We have to use at least N//3 triangles to connect N points. | ||
| simplices = self._sorted_simplices() | ||
| n_start = len(self) // 3 | ||
| n_finish = len(self) | ||
|
|
@@ -154,8 +204,17 @@ def _get_minimum_fully_covering_index_of_simplices(self) -> int: | |
|
|
||
| raise OptimizationFailure("Maybe there are duplicate points?") | ||
|
|
||
| def optimize(self): | ||
| # At least N//3 triangles are needed to connect N points. | ||
| def optimize(self) -> Tuple[NDArray, ArrayLike]: | ||
| """Return the alpha value that allows plotting the shape with the minimum number of triangles. | ||
| Vertices of initial triangulation aren't left uncovered. | ||
|
|
||
| Returns: | ||
| alpha_opt(NDArray): optimized alpha value. | ||
| shape(ArrayLike): shape based on the optimized alpha value. | ||
|
|
||
|
Comment on lines
+208
to
+214
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to mention |
||
| """ | ||
|
|
||
| # We have to use at least N//3 triangles to connect N points | ||
| n_min = self._get_minimum_fully_covering_index_of_simplices() | ||
| alpha_opt = 1 / np.sqrt(self._sorted_circumradii_sw()[n_min]) - 1e-10 | ||
| simplices = self._sorted_simplices() | ||
|
|
@@ -164,48 +223,50 @@ def optimize(self): | |
| return alpha_opt, shape | ||
|
|
||
| def set_mask_at_alpha(self, alpha: float): | ||
| """ | ||
| Set the mask for the alpha shape at the specified alpha value. | ||
| """ | ||
| """Set the mask for the alpha shape based on the given alpha value.""" | ||
| mask = self.get_mask(alpha) | ||
| self.set_mask(mask) | ||
| return self | ||
|
|
||
|
|
||
| def _normalize_points(points: NDArray) -> Tuple[NDArray, NDArray, NDArray]: | ||
| """ | ||
| Normalize points to the unit square, centered at the origin. | ||
| """Normalize points to the unit square, centered at the origin. | ||
|
|
||
| Parameters: | ||
| ----------- | ||
| Args: | ||
| points: array-like, shape(N,2) | ||
| coordinates of the points | ||
|
|
||
| Returns: | ||
| -------- | ||
| points: array, shape(N,2) | ||
| normalized coordinates of the points | ||
| points: array, shape(N,2) | ||
| normalized coordinates of the points | ||
|
|
||
| center: array, shape(2,) | ||
| coordinates of the center of the points | ||
|
|
||
| center: array, shape(2,) | ||
| coordinates of the center of the points | ||
| scale: array, shape(2,) | ||
| scale factors for the normalization | ||
|
Comment on lines
-180
to
+247
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for suggesting an alternative docstring standard. I have been using the numpy standard for years, but your way also looks nice |
||
|
|
||
| scale: array, shape(2,) | ||
| scale factors for the normalization | ||
| """ | ||
|
|
||
| center = points.mean(axis=0) | ||
| scale = np.ptp(points, axis=0) # peak to peak distance | ||
| normalized_points = (points - center) / scale | ||
|
|
||
| return normalized_points, center, scale | ||
|
|
||
|
|
||
| def _circumradius_sq(lengths): | ||
| r""" | ||
| Calculate the squared circumradius `r_c^2`, | ||
| where | ||
| r_c = \frac {abc}{4{\sqrt {s(s-a)(s-b)(s-c)}}} | ||
| See: `https://en.wikipedia.org/wiki/Circumscribed_circle` | ||
| def _circumradius_sq(lengths: NDArray) -> Union[float, np.inf]: | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This type hint raises an exception for python3.8: In later versions it is not a problem, but are you sure we can have a Union with np.inf, which is not a type? |
||
| """Calculate the squared circumradius `r_c^2` where r_c = \frac {abc}{4{\sqrt {s(s-a)(s-b)(s-c)}}}. | ||
| See more about it on: `https://en.wikipedia.org/wiki/Circumscribed_circle`. | ||
|
|
||
| Args: | ||
| lengths(NDArray): contains lengths of triangle's sides. | ||
|
|
||
| Returns: | ||
| Union[float, np.inf]: value of squared circumradius. | ||
|
|
||
| """ | ||
|
|
||
| lengths = np.asarray(lengths) | ||
| s = np.sum(lengths) / 2 | ||
|
|
||
|
|
@@ -219,23 +280,37 @@ def _circumradius_sq(lengths): | |
| return num / denom | ||
|
|
||
|
|
||
| def _calculate_cirumradius_sq_of_triangle(x: ArrayLike, y: ArrayLike): | ||
| """ | ||
| calculates the squared circumradius of a triangle with coordinates x, y | ||
| def _calculate_cirumradius_sq_of_triangle(x: ArrayLike, y: ArrayLike) -> NDArray: | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again, verify type hints |
||
| """Calculate the squared circumradius of a triangle with coordinates x, y. | ||
|
|
||
| Parameters: | ||
| ----------- | ||
| x, y: array-like, shape(3,) | ||
| Args: | ||
| x, y: array-like, shape(3,) | ||
| coordinates of the triangle | ||
|
|
||
| Returns: | ||
| NDArray: squared circumradius of internal triangle. | ||
|
|
||
| """ | ||
|
|
||
| dx = x - np.roll(x, shift=-1) | ||
| dy = y - np.roll(y, shift=-1) | ||
|
|
||
| lengths = np.hypot(dx, dy) | ||
| return _circumradius_sq(lengths) | ||
|
|
||
|
|
||
| def _simplex_to_triangle(smpl, tri): | ||
| def _simplex_to_triangle(smpl: slice, tri) -> Polygon: | ||
| """Return triangle points. | ||
|
|
||
| Args: | ||
| smpl(slice): value of simplex. | ||
| tri: particular triangle. | ||
|
|
||
| Returns: | ||
| Polygon: contains points values of triangle. | ||
|
|
||
| """ | ||
|
|
||
| x = tri.x[smpl] | ||
| y = tri.y[smpl] | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,21 @@ | ||
| """This module contains mechanisms essential to printing figures. | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This script has been changed in main and there are merge conflicts.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for the moment focus on the other one and we can resolve them later |
||
| Functions make necessary operations on points sets and plot appropriate shapes. | ||
| """ | ||
|
|
||
| import numpy as np | ||
| from matplotlib.path import Path | ||
| from matplotlib.patches import PathPatch | ||
|
|
||
|
|
||
| def plot_alpha_shape(ax, alpha_shape): | ||
| """Plot final set of figure's points. | ||
|
|
||
| Args: | ||
| ax(Axes.matplotlib.subplots): axes of image. | ||
| alpha_shape(numpy.ArrayLike): set of points to print. | ||
|
|
||
| """ | ||
|
|
||
| try: | ||
| geoms = alpha_shape.geoms | ||
| except AttributeError: | ||
|
|
@@ -14,10 +26,15 @@ def plot_alpha_shape(ax, alpha_shape): | |
|
|
||
|
|
||
| def _plot_polygon(ax, polygon): | ||
| """Plot a polygon using matplotlib's PathPatch. | ||
| This thread on stackoverflow may be helpful https://stackoverflow.com/a/70533052/6060982. | ||
|
|
||
| Args: | ||
| ax(Axes.matplotlib.subplots): axes of image. | ||
| polygon(shapely.geometry.Polygon): set of points to print. | ||
|
|
||
| """ | ||
| Plot a polygon using matplotlib's PathPatch. | ||
| see https://stackoverflow.com/a/70533052/6060982 | ||
| """ | ||
|
|
||
| xe, ye = polygon.exterior.xy | ||
| exterior = Path(np.column_stack([xe, ye])) | ||
| holes = [Path(np.asarray(hole.coords)) for hole in polygon.interiors] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class definition is self explanatory. No need to add a docstring.