Docstrings and type hints by Rhobar9 · Pull Request #12 · panosz/alpha_shapes

Rhobar9 · 2023-06-09T12:42:22Z

Commits add docstrings and type hints for alpha_shapes.py and plotting.py files. Docstrings follow Google Style Python Docstrings pattern. Please, keep particular attention at type hints. I have checked it many times. However it was most challenging part of task, in few methods flow of object is quite advanced. Maybe I made a mistake somwhere.
I am open for hints and discussion. If need of amendments occurs, I will rewrite code.
Regards, Rhobar9

panosz · 2023-06-12T15:16:34Z

Thank you for this PR. As you may have already noticed, there are some merge conflicts.
Please, add a new commit that resolves them, before we can go on with the review.
Here are some instructions on how you may do this:

Update Your Local Master Branch:

Before you resolve the merge conflict, ensure that your local master branch (or main branch if you're using newer conventions) is up-to-date with the original repository. You can do this by adding the original repository as a remote (if you haven't done this already) and then pulling from it:
```
git remote add upstream git@github.com:panosz/alpha_shapes.git
git fetch upstream
git checkout master
git merge upstream/master
```
Checkout Your Feature Branch:

After you've updated your master branch, checkout your feature branch which has the conflict with the original repository:
```
git checkout docstrings_and_type_hints
```
Merge Your Updated Master Branch Into Your Feature Branch:

Merge your updated master branch into your feature branch:
```
git merge master
```
Now, you are likely to face merge conflicts.
Resolve Merge Conflicts:

Git will give you an output indicating which files are conflicted. Open those files and you'll see something like this:
```
<<<<<<< HEAD
your changes
=======
changes made by others
>>>>>>> commit_id
```
You have to decide which changes to keep. You can keep your changes, their changes, or even both. Once you've resolved the conflict, delete these markers.
Commit Your Resolved Files:

After you've resolved all conflicts, add your resolved files with git add alpha_shapes/alpha_shapes.py. Once all resolved files are added, commit the changes:
```
git commit -m "Resolved merge conflicts"
```
Push Changes to Your Forked Repository:

Finally, push your changes back up to your forked repository:
```
git push origin docstrings_and_type_hints
```

Your PR in the original repository will automatically get updated with your latest push.

Rhobar9 · 2023-06-13T15:48:31Z

Yes, I have seen merge conflicts. I was waiting for your comment and approval on solving them. Thank you for hints. I will solve merge conflicts possibly fast as I can.

Rhobar9 · 2023-06-15T09:11:30Z

I have resolved merge conflicts. Now everything should be fine. I will briefly explain the idea that guided me in sloving the problem.
I did not touch the code responsible for mechanics of objects and project. I copied this part of code from upstream. Overall I was trying copy also your docstrings and comments. I have change them only when it was necessary due to coherency.
We can start review now, I suppose.
Best regards,
Rhobar9

panosz

Hi Rhobar9,

Firstly, thank you for your work in improving the documentation. It's great to see your dedication, and your efforts have been genuinely valuable.

As we continue to work on this, I wanted to share some insights into how we can make our docstrings even more effective.
Ideally, a docstring should explain the purpose or intention of a function rather than the technical details of how it
accomplishes its task. This will make it easier for other developers to understand why a particular function exists and
how it fits into the overall project.

A good example of this practice is the docstring you have added to the 'optimize' method:

def optimize(self) -> (NDArray, ArrayLike):
        """Eliminate redundant simplices and then sets appropriate mask.
        Returns:
            alpha_opt: the most accurate alpha value based on minimal amount of simplices
            shape: shape after optimization
        """

This is a great docstring, as it explains the purpose of the function and what it returns.

In contrast, the docstring you've written for the function is more about what it does internally:

def __init__(self, coords: NDArray) -> None:
        """In try block function invokes __init__ method of class
        Triangulation from matplotlib package and sends
        to it default set of coordinates. If ValueError occurs,
        function will tackle it.
        """

While this is technically correct, it doesn't tell us much about the purpose of the function.
Also, I think it is preferable for __init__ methods to be left withtout docstrings. Instead we can add relative
information to the docstring of the class (see the comment I have added to the relevant lines).

Sometimes, we have classes or functions whose purpose is quite evident from their names. In such cases, the aim of the docstring should be to provide any additional context or explanation that might not be immediately obvious from the name. It should be more generic, outlining its potential usage, rather than detailing a very specific use case.

For example, consider the NotEnoughPoints exception:

class NotEnoughPoints(AlphaException):
    """If instance of class Delaunay has less than 3 points, this exception will be raised."""
    pass

In this case, the name of the exception already gives a good indication of when it might be used, and the docstring seems to limit its usage to just the Delaunay class. It might be more beneficial to provide a general explanation, like so:

class NotEnoughPoints(AlphaException):
    """Raised when an operation requires a certain number of points and that condition is not met."""
    pass

In this way, we maintain the flexibility of our exception, and the docstring serves as a general guideline rather than a strict rule.

Please take a look at the docstrings you have added and see if you can apply these principles to them.
You may find that some of them are already quite good, and don't need to be changed. In other cases, you may find that a
docstring only reiterates what is already clearly stated in the function name. In such cases, it is better to leave the docstring out entirely.

Feel free to reach out if you have any questions or need further clarification.

panosz · 2023-06-19T10:03:09Z

+        """In try block function invokes __init__ method of class
+        Triangulation from matplotlib package and sends
+        to it default set of coordinates. If ValueError occurs,
+        function will tackle it.
+


Ideally, a docstring should explain the purpose or intention of a function rather than the technical details of how it accomplishes its task. Also I think it is best not to add docstrings directly to the init method of any class. Instead, consider adding the relevant info to the class doctstring. One example would be:

""" Parameters: ----------- coords: NDArray, The coordinates of the points (... possibly add more explanations here ...) Raises: ------- ValueError: When so and so occurs

"""

panosz · 2023-06-19T10:06:38Z

+    Delaunay is abstract class, which derives from matplotlib.tri.Triangulation.
+    It adds set of coordinates and essential methods.


First row repeats what is already stated clearly in the code.
2nd row is too vague.
I would prefer if you somehow attempted to retain the original info, i.e. that the intention of this class is to implement a similar interface as scipy.spatial.Delaunay
(I actually cannot recall why this decision was made in the first place. This makes keeping this info in the docstrings especially important)

panosz · 2023-06-19T10:08:35Z



 class NotEnoughPoints(AlphaException):
+    """If instance of class Delaunay has less than 3 points, this exception will be raised."""


In this case, the name of the exception already gives a good indication of when it might be used, and the docstring seems to limit its usage to just the Delaunay class. It might be more beneficial to provide a general explanation, like soIn this case, the name of the exception already gives a good indication of when it might be used, and the docstring seems to limit its usage to just the Delaunay class. It might be more beneficial to provide a general explanation, like so

"""Raised when an operation requires a certain number of points and that condition is not met."""

panosz · 2023-06-19T10:49:49Z

I also noticed that the .idea directory from your local IntelliJ IDEA configuration has been inadvertently committed to the repository. This directory typically contains user-specific settings and should generally not be part of the version-controlled code.

To rectify this, I've updated the .gitignore file in the repository to exclude the .idea directory. Here are the steps you should follow to remove the .idea files from your branch:

Pull the latest changes from the original repository (upstream) to your local master branch:

git checkout master  # switch to your local master branch
git pull upstream master  # pull the latest changes, including the updated .gitignore

If you haven't already, you can add the original repository as "upstream" with this command:

git remote add upstream git@github.com:panosz/alpha_shapes.git

Merge the updated master branch into your docstrings_and_type_hints branch:

git checkout docstrings_and_type_hints  # switch to your feature branch
git merge master  # merge changes from master branch into feature branch

Now, remove the .idea directory from your docstrings_and_type_hints branch:
```
git rm --cached -r .idea  # untracks the .idea directory
```

Commit and push these changes to your fork:

git commit -m "Remove .idea directory from version control"
git push origin docstrings_and_type_hints  # push changes to your remote feature branch on your fork

These steps will ensure that the .idea directory is untracked from Git while still existing on your local machine.

As always, if you have any questions or need further assistance, please let me know.

Rhobar9 · 2023-06-19T18:54:45Z

Hi @panosz!
As always, I really appreciate your hints and rich knowledge. Obviously I will follow them.
I will remove all docstrings from init methods. I will introduce changes to docstrings according your pattern. I enjoy working with you on this project. If needed, I am ready to make further amendments.
Of course I will remove .idea files from my repository. Probably I put these files in wrong directory and then typed git add -A command instead git add -u.
Kind regards

"Rectify_problem_with_redundant_idea_files"

panosz · 2023-07-05T08:29:32Z

Hey @Rhobar9
I just wanted to check how it's going. Are you still pursuing this?

Rhobar9 · 2023-07-06T11:28:56Z

Hi @panosz !
Yes, I am writing last changes. I hope it is only matter of days now.
Thank you for patience and involvement. I am glad of our cooperation. I will finish this PR and make docstrings for another files.
Kind regards:)

panosz · 2023-07-06T12:29:56Z

No pressure!
Take your time and enjoy.

Rhobar9 · 2023-07-07T16:53:02Z

Hi again @panosz !
I am so sorry that I made a little bit mess. Yesterday I badly estimated size of needed work and my free time. Today I finished docs, at least for presentation and further amendments:)
In few spots I made utter changes. Unfortunately sometimes your work may resemble new review. If you notice any code that needs improvement, please let me know without hesitation. I am ready to make amendments until we achieve a positive outcome
Kind regards

panosz

Hi @Rhobar9,

Thank you for your contribution to the project. Your effort in improving the documentation is evident, and it's a valuable addition to the codebase.

I've added a few review comments throughout the code, mainly focusing on improving clarity and consistency in the docstrings. My primary goal is to maintain a coherent style throughout the project and ensure that our documentation is as clear as possible for both developers and users.

Please understand that these suggestions come from a place of wanting to enhance the overall quality of our project. I genuinely appreciate the time and energy you've invested in this contribution.

If you have any questions or concerns about the feedback, or if you'd like to discuss any of the points further, please don't hesitate to reach out.

panosz · 2023-09-15T07:17:59Z

+    Args:
+        x: Contains all x values of triangle's points.
+        y: Contains all y values of triangle's points.
+
+    Returns:
+         outcome:  Contains squared circumradius of internal triangle.


Please, retain info of the expected shape of x and y.
It is also OK to document parameters together, if their particular meaning is clear by their name. In this case, it should be perfectly clear that x refers to the x coordinate and y to the y coordinate.

panosz · 2023-09-15T07:19:57Z

    @property
-    def simplices(self):
+    def simplices(self) -> NDArray:
+        """Create simplices property essential to further operations."""


Properties don't "create" — they return or set values. Consider rephrasing for clarity, eg: "Return the collection of triangles" or something similar

panosz · 2023-09-15T07:26:15Z


-    def __len__(self):
+    def __len__(self) -> int:
+        """Return amount of object's edges."""


clear, but to maintain consistency, it might be worth specifying that it returns the number of simplices

panosz · 2023-09-15T07:32:59Z

+    """Abstract class, which provides useful interface. This idea is similar to scipy.spatial.Delaunay solution.
+    Coordinates of future Alpha_Shaper object will be added via Delaunay class.
+    If the coordinates do not meet the conditions, the class will raise an appropriate error.
+    It also adds key methods.
+


Consider rephrasing, for conciseness.
Consider documenting the coords parameter of __init__.
Also, try to standardize the info on when initialization raises an exception with the following pattern

""" Raises: - NotEnoughPoints: If there are fewer than 3 points provided. - ValueError: For other value-related issues with the coordinates. """

I would be extra grateful, if you tried to incorporate here any info of when the base class raises an exception

panosz · 2023-09-15T07:37:06Z

+    """Crucial class to creating alpha-shapes. 
+   The class handles points, creates internal triangles and generates shapes. 


Please, try to document the parameters of init.
Info about normalize can be found in README and example scripts.

panosz · 2023-09-15T08:01:46Z

+        """Eliminate redundant simplices and then set appropriate mask.
+
+        Returns:
+            alpha_opt: the most appropriate alpha value based on minimal amount of simplices.
+            shape: shape after optimization.


This is a bit inaccurate. From where are the redundant simplices eliminated?
As it is, the docstring suggests that the method has side effects, i.e. somehow modifies the state of the object. Is this correct? What does this method return?

panosz · 2023-09-15T08:03:43Z

+    """Calculate the squared circumradius of triangle.
+    See more about it on: `https://en.wikipedia.org/wiki/Circumscribed_circle`.


Please, keep latex formula.

panosz · 2023-09-15T08:04:10Z

+    Args:
+        lengths: contains lengths of triangle's sides.
+
+    Returns:
+        Contains values of squared circumradiuses.


Consider documenting the type and shape of the arguments and output

panosz · 2023-09-15T08:07:58Z

+    """Create internal triangle from given simplices.
+


It is not clear what an "internal triangle" is. Consider rephrasing, for clarity.

panosz · 2023-09-15T08:11:35Z



 def plot_alpha_shape(ax, alpha_shape):
+    """Mainline function, which plots all sets of figure's points.


In general, I find that it is best to avoid characterizations such as "mainline", "central" and so on in docstrings, unless there is really good reason to do it.

Rhobar9 · 2023-11-05T19:17:49Z

I have just pushed the code. I will describe my doubts and the scope of work in detail in the issue discussion.

panosz

Thank you for your contribution.
Please take a look at the comments in the review.

panosz · 2023-11-07T11:09:53Z



 class AlphaException(Exception):
+    """Abstract class for exceptions which could be raised during the work of Alpha_Shaper class."""


This class definition is self explanatory. No need to add a docstring.

Remember docstrings come at a cost. They add extra visual info. In this case, I think it should be avoided, because it offers no extra info other than what the reader might have guessed at a glance, if the docstring was not there.

panosz · 2023-11-07T11:14:22Z


-class OptimizationWarnging(UserWarning):
+class OptimizationWarning(UserWarning):
+    """Warns user without interrupting the program."""


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.

panosz · 2023-11-07T11:16:52Z


 class Delaunay(Triangulation):
-    """
+    """Abstract class with useful interface.


Avoid using the term "abstract class", unless you are using abstract base classes and feel that you need to communicate this to the user

panosz · 2023-11-07T11:19:39Z

+    """Abstract class with useful interface.
    Visitor sublclass of matplotlib.tri.Triangulation.
-    Mimics scipy.spatial.Delaunay interface.
+    See similar idea on scipy.spatial.Delaunay solution.


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)

panosz · 2023-11-07T11:20:40Z

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


Nice. Try hinting the user about the expected shape of the coords parameter

panosz · 2023-11-07T11:57:08Z

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


No need to mention plotting. Although plotting the alpha shape is an obvious option, we don't want to assume too much about the users' intentions. Focus on what this method does, not on how we think its output should be used.

panosz · 2023-11-07T11:59:22Z

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


Thank you for suggesting an alternative docstring standard. I have been using the numpy standard for years, but your way also looks nice

panosz · 2023-11-07T12:03:42Z

-    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]:


This type hint raises an exception for python3.8:

File "examples/alpha_shape_exmpl.py", line 4, in <module> from alpha_shapes import Alpha_Shaper, plot_alpha_shape File "/home/panosz/Documents/programming/python/alpha_shapes/alpha_shapes/__init__.py", line 1, in <module> from .alpha_shapes import Alpha_Shaper File "/home/panosz/Documents/programming/python/alpha_shapes/alpha_shapes/alpha_shapes.py", line 259, in <module> def _circumradius_sq(lengths: NDArray) -> Union[float, np.inf]: File "/usr/lib/python3.8/typing.py", line 261, in inner return func(*args, **kwds) File "/usr/lib/python3.8/typing.py", line 358, in __getitem__ parameters = tuple(_type_check(p, msg) for p in parameters) File "/usr/lib/python3.8/typing.py", line 358, in <genexpr> parameters = tuple(_type_check(p, msg) for p in parameters) File "/usr/lib/python3.8/typing.py", line 149, in _type_check raise TypeError(f"{msg} Got {arg!r:.100}.") TypeError: Union[arg, ...]: each arg must be a type. Got inf.

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?

panosz · 2023-11-07T12:16:09Z

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


again, verify type hints

panosz · 2023-11-07T12:18:33Z

@@ -1,9 +1,21 @@
+"""This module contains mechanisms essential to printing figures.


This script has been changed in main and there are merge conflicts.

for the moment focus on the other one and we can resolve them later

Rhobar9 and others added 4 commits May 29, 2023 13:40

added_docs_for_exceptions_and_Delaunay_class

49d8890

added_docs_and_type_hints_for_Alpha_Shaper_class

a99fd0a

added_docs_for_all_objects_in_plotting.py_file

1f20e04

last_amendments

c2a54f4

Resolved merge conflicts

46febc3

panosz reviewed Jun 19, 2023

View reviewed changes

Aleksander Koprowski added 2 commits June 20, 2023 17:15

Merge branch 'master' into docstrings_and_type_hints

9db83f8

"Rectify_problem_with_redundant_idea_files"

Remove .idea directory from version control

d273e36

Amendments_after_first_review_

9336180

panosz reviewed Sep 15, 2023

View reviewed changes

Amendments_after_second_review

7b2d80d

panosz requested changes Nov 7, 2023

View reviewed changes

		Delaunay is abstract class, which derives from matplotlib.tri.Triangulation.
		It adds set of coordinates and essential methods.



		class NotEnoughPoints(AlphaException):
		"""If instance of class Delaunay has less than 3 points, this exception will be raised."""

		"""Crucial class to creating alpha-shapes.
		The class handles points, creates internal triangles and generates shapes.

		"""Calculate the squared circumradius of triangle.
		See more about it on: `https://en.wikipedia.org/wiki/Circumscribed_circle`.



		def plot_alpha_shape(ax, alpha_shape):
		"""Mainline function, which plots all sets of figure's points.



		class AlphaException(Exception):
		"""Abstract class for exceptions which could be raised during the work of Alpha_Shaper class."""

		@@ -1,9 +1,21 @@
		"""This module contains mechanisms essential to printing figures.

Conversation

Rhobar9 commented Jun 9, 2023

Uh oh!

panosz commented Jun 12, 2023

Uh oh!

Rhobar9 commented Jun 13, 2023

Uh oh!

Rhobar9 commented Jun 15, 2023

Uh oh!

panosz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

panosz commented Jun 19, 2023

Uh oh!

Rhobar9 commented Jun 19, 2023

Uh oh!

panosz commented Jul 5, 2023

Uh oh!

Rhobar9 commented Jul 6, 2023

Uh oh!

panosz commented Jul 6, 2023

Uh oh!

Rhobar9 commented Jul 7, 2023

Uh oh!

panosz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rhobar9 commented Nov 5, 2023

Uh oh!

panosz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

panosz left a comment •

edited

Loading