Skip to content
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

Scalar creation function names #58

Open
benbovy opened this issue Oct 21, 2024 · 4 comments
Open

Scalar creation function names #58

benbovy opened this issue Oct 21, 2024 · 4 comments

Comments

@benbovy
Copy link
Owner

benbovy commented Oct 21, 2024

  • 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 #51 (comment)

@jorisvandenbossche
Copy link
Collaborator

Summarizing the background and current status here:

Originally we had with spherely.Point(..), spherely.Linestring(..) and spherely.Polygon(...) constructors (and #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 #51 in favor of a single spherely.Geography class that all objects of any dimensionality use.

#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 (#51 (comment)) 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):

  • Keep the current point(), linestring(), polygon() etc functions
  • Rename those to use a prefix (create_point(), create_linestring(), create_polygon(), etc, or make_point(), make_linestring(), make_polygon() etc)
  • Actually match shapely regardless of not having subclasses, and capitalize the functions (i.e. Point() as a function to create a Geography object of type POINT, etc), although Python style-wise that is a bit strange.
  • Something else?

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

@martinfleis
Copy link
Contributor

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
Copy link
Owner Author

benbovy commented Oct 22, 2024

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants