jmespadero / pyDelaunay2D

A simple Delaunay 2D triangulation in python (with numpy)
GNU General Public License v3.0
191 stars 34 forks source link

Questions about magic(?) constant and random(?) naming convention #1

Closed 1kastner closed 7 years ago

1kastner commented 7 years ago

First of all thanks for the great project! I started to read the code and wanted to re-use it for my own purposes. I am new to Delaunay triangulation so please bear with me. As you said that this is for educational purposes, I hope my remarks can be even helpful for other learners, too.

While reading I got the following questions

dt = Delaunay2D(center, 50 * radius)

Why did you choose 50 as a magic constant here? Is there any specific reason to it?

Delaunay2D.exportDT(self)

First of all, I realized you did not stick to the PEP8 naming conventions of Python and I did not understand, when you start a method name with a capitalized letter, e. g. Delaunay2D.AddPoint(self, p) and when with a lowercase letter, as in Delaunay2D.exportDT(self). This makes reading the code quite confusing. Especially because according to PEP8 method names should be snailcase, i.e. add_point(self, point) or export_delaunay_triangulation(self). Is there a special system you used so that these different naming styles actually have a meaning?

Second, it could be really helpful if there were some short examples of what I should invoke a method with and what I should expect back. What is the exact difference between Delaunay2D.exportDT(self) and Delaunay2D.exportTriangles(self)? When I look at the comments or the code, I get an idea of what might be going on there, but the method name does not help much.

jmespadero commented 7 years ago

Hi,

I used the value 50 * radius, but any arbitrary number greater than 2 * radius should work. The Bowyer-Watson algorithm uses a "super frame", which is a box able to contain you input set of point. You want the corners of the box to stay far of your input points, so you have no problem in deciding which triangle contain your current point. This is because if the frame is "tight" (as when using the Axis Aligned Bounding Box), you can obtain degenerate triangles (which area near to zero) and your code is exposed to fail in the next point-inside-triangle-circumcircle test.

For naming convention... I did not put too much effort to stick PEP8. Probably I will correct that soon, so thanks for the issue.

The difference is that

1kastner commented 7 years ago

Ok thanks a lot for your explanation!

jmespadero commented 7 years ago

I have restyled the code to be more PEP8 compliant. It is not 100% PEP8 but, should look better now.

1kastner commented 7 years ago

thanks, much more readable to me!