Fix and rename: get_dimensions()

[benbovy]

Closes #57.

• Return values that are consistent with shapely for collections.
• Rename get_dimensions to get_dimension
• Remove properties Geography.dimensions and Geography.nshape.

[benbovy]

Note: the Geography.dimensions property still returns the results from the underlying s2geography::Geography::dimension() so it is inconsistent with spherely.get_dimensions().

I wonder if we shouldn't remove the Geography dimensions and nshape altogether...

[paleolimbot]

I wonder if we shouldn't remove the Geography dimensions and nshape altogether...

(It was probably a bad name in s2geography...it's useful to know if everything is a single dimension, but easy to get confused with s2_dimension().)

[benbovy]

Sorry it wasn't clear, I meant remove those properties from the Python public API.

(And maybe expose another function to besides get_dimensions() to expose both behaviors?)

[jorisvandenbossche]

I think it would indeed be good to remove that attribute for now, because it probably will only be confusing (and we can always add it later again if we want to do so)

FWIW, I think the name of this function should actually be get_dimension, i.e. without the last s (not plural). It's how this is in shapely (and so there is an argument for consistency with shapely), but it seems a misnomer there, because all other similar functions use a singular term (get_type_id, get_coordinate_dimension, etc)

[benbovy]

I think the name of this function should actually be get_dimension, i.e. without the last s (not plural).

Agreed. non-plural form makes more sense and is used in R's s2 and bigquery.

Maybe we could have a temporary alias here for compatibility with Shapely? And at the same time try to suggest changing the name of the function in shapely (with some deprecation cycle)?

[jorisvandenbossche]

And at the same time try to suggest changing the name of the function in shapely (with some deprecation cycle)?

Yeah, I was also thinking that and would be supportive of at least adding such an alias in shapely (we can still see if we actually deprecate the other long term).

If we would add an alias on the shapely side, not sure if it's needed to have both aliases here, though (on the short term, I don't think anyone is going to write code where they expect they can exactly swap shapely. with spherely. and have working code)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.10%. Comparing base (38a0693) to head (0672f27).
Report is 44 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #87      +/-   ##
==========================================
- Coverage   94.00%   88.10%   -5.90%     
==========================================
  Files           5       13       +8     
  Lines         200      782     +582     
  Branches        8       63      +55     
==========================================
+ Hits          188      689     +501     
- Misses          5       63      +58     
- Partials        7       30      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.