flatsurf / sage-flatsurf

Flat surfaces in Sage
https://flatsurf.github.io/sage-flatsurf/
GNU General Public License v2.0
10 stars 10 forks source link

Improve naming of polygons #229

Open saraedum opened 1 year ago

saraedum commented 1 year ago

Currently, we are exposing these names in our __init__.py:

from .geometry.polygon import polygons, EquiangularPolygons, Polygons, ConvexPolygons

The semantic of these objects are quite confusing (at least they've been to me in the past).

polygons has a bunch of methods rectangle, regular_ngon, … but at the same time it can be called

polygons((1, 2), (-3, -4), (2, 2))

Note that the parameters are edges here (it always takes me a moment to figure this one out.) But it's really confusing to me that this is called polygons and not polygon in this example.

Then Polygons and ConvexPolygons are SageMath parents. (Imho, these should be categories, fixed in #220.) So they are called with a ring and then the actual parent is called with edges to create an actual polygon. (There's something wrong with convexity here but that's fixed in #220.)

Finally, EquiangularPolygons are not a parent but a "factory factory". Calling it with angles, creates a polygon factory. Calling it with side lengths, then creates an actual polygon.


I am not entirely sure what's the correct setup here. But here's a proposal:

There should be a callable polygon. I propose to have it be:

def polygon(*, vertices=None, edges=None, +other bits such as base ring)

So we always have to be explicit about whether we are passing in vertices or edges.

The example polygons square, triangle, regular_ngon, rectangle, right_triangle should just be methods in flatsurf.geometry.polygon but be exposed in flatsurf directly. I don't see the benefit of hiding them under the polygons "namespace" that is not actually a module.

Polygons and ConvexPolygons become categories in #220. They are still exposed as Polygons and ConvexPolygons for backwards compatibility but internally they are in flatsurf.geometry.categories.real_projective_polygons. They can now be obtained "correctly" as RealProjectivePolygons(ring) and RealProjectivePolygons(ring).Convex() [though I don't see why anybody would want to create the latter.]

Finally, EquiangularPolygons becomes RealProjectivePolygons(ring).WithAngles(…) so it's no different from all the other polygon categories. (Note that equiangular is misleading. For me at least equiangular is one of the two ingredients of regular = equiangular + equilateral.)