google / s2geometry

Computational geometry and spatial indexing on the sphere
http://s2geometry.io/
Apache License 2.0
2.32k stars 308 forks source link

Python: Add repr() methods #164

Open rcoup opened 3 years ago

rcoup commented 3 years ago

Add's repr() methods to some core cell/geodetic Python classes.

Goal is to make debugging & interactive exploration easier.

<S2LatLng: (51.336860,0.493198)>
<S2LatLngRect: (51.3368602,0.4931979,51.7323965,0.1495211)>
<S2CellId: 2/100323000212021010333002003201>
<S2Cell: 2/100323000212021010333002003201>

(based on #160, only the last commit actually has any changes. Rebase once that's merged)

jmr commented 3 years ago

Based on my current understanding of str and repr, str is for debugging/humans, and repr should give an unambiguous result, preferably able to be evaled. So I'd say use operator<< for str and have repr look like:

S2LatLng.FromDegrees(51.336860, 0.493198)
S2LatLngRect(S2LatLng.FromDegrees(51.3368602, 0.4931979), S2LatLng.FromDegrees(51.7323965, 0.1495211))
S2CellId(0x...)
S2Cell(S2CellId(0x...))
rcoup commented 3 years ago

With some limited exceptions (mostly in stdlib), repr() strings are virtually never eval-able. The main purpose is to provide developers with clear information as to what object they have for debugging.

Currently repr() on S2 objects returns eg:<pywraps2.S2Point; proxy of <Swig Object of type 'S2Point *' at 0x7fc317469f60> >, which isn't any use.

As you said in a comment on #160, 2/100323000212021010333002003201 is actually the most useful debugging string for S2 cells. Apart from seeing two are the same (though == works) seems like there's nothing you can do or interpret with the 64-bit numbers.

Similarly I think the representations for LatLng classes just need to be as clear and simple as possible.

jmr commented 3 years ago

https://docs.python.org/3/reference/datamodel.html#object.__repr__

If at all possible, this should look like a valid Python expression that could be used to recreate an object with the same value (given an appropriate environment). If this is not possible, a string of the form <...some useful description...> should be returned.

Seems like a pretty official recommendation.

As you said in a comment on #160, 2/100323000212021010333002003201 is actually the most useful debugging string for S2 cells.

I hadn't read up on __repr__ yet. Now I think that's what __str__ should return.

rcoup commented 3 years ago

Seems like a pretty official recommendation.

Sure, but it's impossible to implement in many classes, yet every class really needs a repr() method (humans aren't so great at deciphering content from a memory address).

Following the recommendation logic, would a S2Polygon with 10K vertices return a 250KB string of coordinates? No, but where does that line get arbitrarily drawn? Obviously anything where the state isn't completely available & deterministic via the constructor (eg: S2RegionCoverer) isn't possible to do as an expression. IME most Python codebases move towards consistency for __repr__() methods instead of building valid python expressions in a few cases & not others.

As you said in a comment on #160, 2/100323000212021010333002003201 is actually the most useful debugging string for S2 cells.

I hadn't read up on __repr__ yet. Now I think that's what __str__ should return.

__str__() returns __repr__() by default fwiw.

S2LatLng.FromDegrees(51.336860, 0.493198)
S2LatLngRect(S2LatLng.FromDegrees(51.3368602, 0.4931979), S2LatLng.FromDegrees(51.7323965, 0.1495211))
S2CellId(0x...)
S2Cell(S2CellId(0x...))

Anyway, at the end of the day it's your library 😄 . If you want it I can easily swap __repr__() to those ^ and put the others into __str__().

jmr commented 3 years ago

Following the recommendation logic, would a S2Polygon with 10K vertices return a 250KB string of coordinates?

This is how dict and list work. They don't truncate anything.

Based on what I currently know, I would say make repr evalable (or just leave it alone) and str can use the C++ debug string functions.

Is there a moderately well respected Python style guide that discusses repr/str? I only found random stackoverflow posts.

rcoup commented 3 years ago

Is there a moderately well respected Python style guide that discusses repr/str? I only found random stackoverflow posts.

Not that I can find or know of. Django is a large, mature and well documented framework, this is a summary of their __repr__() methods fwiw. OTOH SQLAlchemy seems to try and have Foo(x=y) style more (lots of their search results are in examples/ though).

Based on what I currently know, I would say make repr evalable (or just leave it alone) and str can use the C++ debug string functions.

ok. eval-able is better than a memory address.