benbovy / spherely

Manipulation and analysis of geometric objects on the sphere.
https://spherely.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
114 stars 8 forks source link

Scalar creation function names #58

Open benbovy opened 1 week ago

benbovy commented 1 week ago
  • Scalar creation function naming: still no strong opinion, although I would lean towards either point(), linestring(), etc. or create_point(), create_linestring(), etc.

Let's open a separate issue for this and see if we can get some more opinions

Originally posted by @jorisvandenbossche in https://github.com/benbovy/spherely/issues/51#issuecomment-2422496943

jorisvandenbossche commented 1 week ago

Summarizing the background and current status here:

Originally we had with spherely.Point(..), spherely.Linestring(..) and spherely.Polygon(...) constructors (and https://github.com/benbovy/spherely/pull/26 was then adding additional constructors for the Multi.. variants). This exactly mimicked the Shapely class constructors. However, we decided for now that we don't actually need those subclasses, which were removed https://github.com/benbovy/spherely/pull/51 in favor of a single spherely.Geography class that all objects of any dimensionality use.

https://github.com/benbovy/spherely/pull/51 then replaced the class constructors with equivalent functions: spherely.point(..), spherely.linestring(..) and spherely.polygon(..), and added multipoint/multilinestring/multipolygon/collection as well. Important to note (comparing to shapely) that those are just scalar constructor functions. For vectorized constructors we for now only have spherely.points(..) to create an array of point geographies from lon/lat coordinates (it's another question to what extent we need those other vectorized creation functions).

On the PR, I brought up (https://github.com/benbovy/spherely/pull/51#issuecomment-2407697973) that given we no longer strictly match shapely with the subclass constructors, we could also consider other naming schemes.

For example, inspired on the naming of R s2 and bigquery, we could use something like make_point() instead of point(), etc (although they are also not consistent in it, as they have st_makeline/st_makepolygon but st_geogpoint ... https://r-spatial.github.io/s2/reference/s2_geog_point.html) Or when using prefixes, we could also go with create_.. functions.

The prefix makes it longer to type (and especially for point vs create_point maybe also harder to read, given the arguments in this case are typically short, so making the overall code considerably longer), but on the plus side then tab completion will put them all together (and there is a clearer distinction from being a type).

So summarizing the current options (there might be others though):

Personally I don't have a strong opinion (or clear preference) at the moment. Thoughts? (cc @martinfleis @brendan-ward)

martinfleis commented 1 week ago

I don't have a strong opinion either. One thing I would avoid is capitalisation of function names as that comes with wrong expectations (which is that speherely.Point() create a class called Point.

I kind of like the create_ option due to nice grouping on tab completion but the option without any prefix will work just fine as well. If we use the prefix, it is easier to signal to users that this is not expected to mirror shapely's behaviour so it may be worth doing that.

benbovy commented 1 week ago

Thanks @jorisvandenbossche for the detailed summary!

I also agree that we should avoid capitalisation.

+1 for a create_ prefix (I slightly prefer it over make_). It is a bit longer to type but I don't find it really annoying. It also makes the function name even less ambiguous than without prefix, and it is usual to have such prefix for similar functions in the Python ecosystem.

martinfleis commented 1 week ago

Yeah, on the create_ vs make_ I also prefer the former.