search

sidewalklabs / s2sphere

Python implementation of the S2 geometry library.
http://s2sphere.sidewalklabs.com/
MIT License
213 stars 44 forks source link

implement CellId.walk() and CellId.walk_fast() #10

Closed svenkreiss closed 8 years ago

svenkreiss commented 8 years ago

more pythonic iterator over cells at a given level

This change is Reviewable

danvk commented 8 years ago

A few comments. At a higher level, do we want to preserve the 1↔1 mapping between C++ and Python classes? If so, walk and walk_fast should be provided in a separate module.

Previously, svenkreiss (Sven Kreiss) wrote… > #### implement CellId.walk() and CellId.walk_fast() > > more pythonic iterator over cells at a given level > >

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.

s2sphere/sphere.py, line 1139 [r1] (raw file):

            Iterator over instances of :class:`CellId`s.
        """
        cellid = cls.begin(level).id()

Python question: why do cls.begin rather than CellId.begin?

_s2sphere/sphere.py, line 1139 [r1] (raw file):_

            Iterator over instances of :class:`CellId`s.
        """
        cellid = cls.begin(level).id()

The naming here is a bit confusing -- is cellid a CellId or an int?

s2sphere/sphere.py, line 1141 [r1] (raw file):

        cellid = cls.begin(level).id()
        endid = cls.end(level).id()
        increment = cls.begin(level).lsb() << 1

This is cryptic; add a comment (also in walk_fast)

s2sphere/sphere.py, line 1154 [r1] (raw file):


        Use with caution: This modifies the underlying ``id`` of a single
        :class:`CellId` instance for performance reasons.

I'd rewrite this comment to be more oriented towards a user of this function (and less about implementation details). For instance, something like

Use with caution: this repeatedly yields mutates a single object with a changing ``id``.
If you save the object, it will change out from underneath you.

Comments from Reviewable

svenkreiss commented 8 years ago

Thanks for the comments.

That 1 to 1 mapping never existed. This was one of the comments from the original author that he regretted not having preserved that better.

Previously, danvk (Dan Vanderkam) wrote… > A few comments. At a higher level, do we want to preserve the 1↔1 mapping between C++ and Python classes? If so, `walk` and `walk_fast` should be provided in a separate module.

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.

s2sphere/sphere.py, line 1139 [r1] (raw file):

Previously, danvk (Dan Vanderkam) wrote… > Python question: why do `cls.begin` rather than `CellId.begin`?

This has something to do with the behavior for derived classes. I had never used it like this before (I usually use @staticmethod and not @classmethod), but I think it could make sense so that begin is called of the derived class in case it is overwritten.

_s2sphere/sphere.py, line 1139 [r1] (raw file):_

Previously, danvk (Dan Vanderkam) wrote… > The naming here is a bit confusing -- is `cellid` a `CellId` or an `int`?

an int. I am struggling to come up with a better name.

s2sphere/sphere.py, line 1141 [r1] (raw file):

Previously, danvk (Dan Vanderkam) wrote… > This is cryptic; add a comment (also in `walk_fast`)

Done.

s2sphere/sphere.py, line 1154 [r1] (raw file):

Previously, danvk (Dan Vanderkam) wrote… > I'd rewrite this comment to be more oriented towards a user of this function (and less about implementation details). For instance, something like > > `````` > Use with caution: this repeatedly yields mutates a single object with a changing ``id``. > If you save the object, it will change out from underneath you. > ```

Done.

Comments from Reviewable

danvk commented 8 years ago

Some more suggestions for the comments, otherwise :lgtm:

Previously, svenkreiss (Sven Kreiss) wrote… > Thanks for the comments. > > That 1 to 1 mapping never existed. This was one of the comments from the original author that he regretted not having preserved that better.

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.

_s2sphere/sphere.py, line 1139 [r1] (raw file):_

Previously, svenkreiss (Sven Kreiss) wrote… > an `int`. I am struggling to come up with a better name.

cell_id_int?

s2sphere/sphere.py, line 1141 [r1] (raw file):

Previously, svenkreiss (Sven Kreiss) wrote… > Done.

This still wasn't clear to me... @svenkreiss and I hashed it out. How about something like this?

The 64-bit ID has:
- 3 bits to encode the face
- 0-60 bits to encode the position
- a 1

The final 1 is the least significant bit. Doubling it yields the increment between positions as 64-bit IDs.

_s2sphere/sphere.py, line 1154 [r1] (raw file):_

Previously, svenkreiss (Sven Kreiss) wrote… > Done.

You've got "use with caution" twice.

Comments from Reviewable

svenkreiss commented 8 years ago

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

s2sphere/sphere.py, line 1154 [r1] (raw file):

Previously, danvk (Dan Vanderkam) wrote… > You've got "use with caution" twice.

Done.

Comments from Reviewable

danvk commented 8 years ago

:shipit:

Previously, danvk (Dan Vanderkam) wrote… > Some more suggestions for the comments, otherwise :lgtm:

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable