Added 2 helper functions lonlat and latlon, added documentation for each by paulefoe · Pull Request #282 · geopy/geopy

paulefoe · 2018-04-13T11:18:24Z

this commit fixes #202

KostyaEsmukov

First of all, thank you very much for this! Some changes are definitely required, and, TBH, I'm not quite sure how should this look in the end. I left a suggestion on removing latlon function, because it fully replicates Point constructor.

Also I've noticed that these functions are not referenced in the docs. Could you do that please? I think that these should go right after module doc, before geodesic (docs/index.rst file). See CONTRIBUTING.md for how-to on building and viewing docs locally.

PS I know how hard this is to write good docs, especially on a foreign language. I'm still learning here too! Just want to let you know that I appreciate your efforts.

KostyaEsmukov · 2018-04-17T06:19:50Z

    >>> _, pa = g.geocode('Palo Alto, CA')
    >>> print((d(ne, cl) + d(cl, wa) + d(wa, pa)).miles)
    3277.30439191
-


Could you please revert this change?

KostyaEsmukov · 2018-04-17T06:24:11Z

+    >>> newport_ri = (41.49008, -71.312796)
+    >>> cleveland_oh = (41.499498, -81.695391)
+    >>> print(distance(latlon(*newport_ri), latlon(*cleveland_oh)).miles)
+    538.3904453677203


Parameters (below) must be separated from description (above) with one empty line:

""" ... description ... <-- empty newline :param ... """

KostyaEsmukov · 2018-04-17T06:25:29Z

+def latlon(y, x, z=0):
+    """
+    Helper function that can be used for more explicit and obvious calling of the distance methods
+    >>> from geopy.distance import latlon, distance


I believe that code examples must be indented to be rendered correctly, though I'm not sure. Anyway, all the examples in this file are indented, so could you please do that there too?

Still relevant

KostyaEsmukov · 2018-04-17T06:29:38Z

+    Helper function that can be used for more explicit and obvious calling of the distance methods
+    >>> from geopy.distance import latlon, distance
+    >>> newport_ri = (41.49008, -71.312796)
+    >>> cleveland_oh = (41.499498, -81.695391)


IMO it might be not easy to comprehend that this is an y-x order, but this is crucial for understanding the intent of this function. Maybe a more explanatory variable name would help?

newport_ri_yx = (41.49008, -71.312796)

KostyaEsmukov · 2018-04-17T06:38:43Z

+
+def lonlat(x, y, z=0):
+    """
+    Helper function that can be used for more explicit and obvious calling of the distance methods


I don't think that obvious is a proper term for documentation :-)

This description should introduce the reader to the problem: distance accepts coordinates in (y, x)/(lat, lon) order, while some other libraries/systems might use (x, y)/(lon, lat) instead.

And then suggest a solution: using this function, which accepts coordinates in (x, y, [z]) order and returns a Point instance for the distance.

KostyaEsmukov · 2018-04-17T06:43:37Z

+
+def latlon(y, x, z=0):
+    """
+    Helper function that can be used for more explicit and obvious calling of the distance methods


This function is very different from the previous one, so should be the description. I believe it will be confusing to the reader to see a function which transforms (y, x, [z]) to the same (y, x, [z]).

Actually I'm starting to think that instead of introducing this function, it would probably be better to simply encourage the use of the Point constructor instead. What do you think?

paulefoe · 2018-05-02T21:13:00Z

Hey, it's been a while, I'm verry sorry. I got sick and then a lot of work appeared suddenly.
Anyway, so should I delete latlon function?
And did build failed? I don't get it, I didn't change anything other than docs.

merge

merge from origin

KostyaEsmukov · 2018-05-04T19:23:06Z

Hey, it's been a while, I'm verry sorry. I got sick and then a lot of work appeared suddenly.

No problem, this can wait.

Anyway, so should I delete latlon function?

Yup, I think that would be better.

And did build failed? I don't get it, I didn't change anything other than docs.

Click on the red cross near a commit to see the build status #374139588. There's a single failing job #374139594, which is, apparently, caused by a random failure due to the tests relying on the network, which might fail (in this case there was a timeout). The failed tests in this job are surely unrelated to your changes, so all that was needed was to trigger the build one more time. That last merge of yours did it. You could've also amended the commit and force-pushed it.

KostyaEsmukov · 2018-05-04T19:25:27Z

 def lonlat(x, y, z=0):
    """
-    Helper function that can be used for more explicit and obvious calling of the distance methods
+    distance accepts coordinates in (y, x)/(lat, lon) order, while some other libraries/systems might use 


A sentence should start with a capital letter :). But in this case distance is a term (a class reference, more specifically), not a plain word. See the docs around for examples on how distance is formatted there.

Please try to keep the line lengths close to 80-ish.

Coordinate pairs should also be formatted as terms (with backticks, I guess), otherwise they might be split by word-wrapping.

KostyaEsmukov · 2018-05-04T19:39:41Z

+    distance accepts coordinates in (y, x)/(lat, lon) order, while some other libraries/systems might use 
+    (x, y)/(lon, lat).
+    This helper function introduced here for solving this problem. It accepts coordinates in a form of (x, y)/(lon, lat)
+    and returns Point that is safe to use with distance


A sentence should end with a dot (or some other relevant punctuation mark).

It seems to me that these two sentences are intended to be in their own paragraph. Paragraphs should be separated with one empty line, which is missing there.

These two sentences don't sound natural to me. Let me propose the following phrasing:

This function provides a convenient way to convert coordinates of the ``(x, y)/(lon, lat)`` format to a :class:`geopy.point.Point` instance. Example::

…ged docs

paulefoe · 2018-05-07T15:08:34Z

Hey again, thanks for positive feedback, I am doing my best, but it's actually pretty hard to write meaningful docs, so thank you very much for your guidance and patience!

KostyaEsmukov

Just a few notes, otherwise it looks good. Thank you for your efforts!

KostyaEsmukov · 2018-05-08T13:03:57Z

    >>> print((d(ne, cl) + d(cl, wa) + d(wa, pa)).miles)
    3277.30439191
-
+    


This change should be reverted

KostyaEsmukov · 2018-05-08T13:04:20Z

+
+    :param x: longitude 
+    :param y: latitude
+    :param z: altitude


:param z: (optional) altitude

KostyaEsmukov · 2018-05-08T13:05:25Z

+
+    Example::
+
+    >>> from geopy.distance import lonlat, distance


Code blocks should be indented.

Like this: https://github.com/svalee/geopy/blob/f4d0b509dccc1f8a937e1ae2416066bcf611f08b/geopy/distance.py#L289

KostyaEsmukov · 2018-05-08T13:05:54Z

+    Example::
+
+    >>> from geopy.distance import lonlat, distance
+    >>> newport_ri = (-71.312796, 41.49008)


newport_ri_xy and cleveland_oh_xy

KostyaEsmukov

Thank you! Merging.

KostyaEsmukov · 2018-05-10T06:58:40Z

We seem to have forgotten about the tests! Added in fdfaac7.

Also the import in the docs was incorrect: it was

.. autofunction:: geopy.lonlat

... which yielded an error when building the docs, because lonlat is not exported from geopy. It should've been this:

.. autofunction:: geopy.distance.lonlat

I guess it was just a typo.

svalee and others added 3 commits April 13, 2018 14:09

added 2 helper functions lonlat and latlon, added documentation for each

5992118

changed docs

aff9e95

lonlat docs now using lonlat function

5f96b3e

KostyaEsmukov requested changes Apr 17, 2018

View reviewed changes

changed docs

7818ede

svalee added 2 commits May 4, 2018 11:10

Merge branch 'master' of https://github.com/geopy/geopy

eac48a2

merge

Merge branch 'master' of https://github.com/svalee/geopy

ba8ff5d

merge from origin

KostyaEsmukov reviewed May 4, 2018

View reviewed changes

KostyaEsmukov added this to the 1.14 milestone May 6, 2018

removed latlon function because it replicated Point constructor, chan…

f4d0b50

…ged docs

KostyaEsmukov reviewed May 8, 2018

View reviewed changes

added indent to examples, marked optional parameter

578794e

KostyaEsmukov approved these changes May 8, 2018

View reviewed changes

KostyaEsmukov merged commit 2064f7f into geopy:master May 8, 2018

KostyaEsmukov pushed a commit that referenced this pull request May 8, 2018

Add distance.lonlat function (#282)

2dfcf2c

KostyaEsmukov mentioned this pull request May 8, 2018

Distance calculation takes (y, x) instead of (x, y) #202

Closed

		>>> print((d(ne, cl) + d(cl, wa) + d(wa, pa)).miles)
		3277.30439191

Conversation

paulefoe commented Apr 13, 2018

Uh oh!

KostyaEsmukov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulefoe commented May 2, 2018

Uh oh!

KostyaEsmukov commented May 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulefoe commented May 7, 2018

Uh oh!

KostyaEsmukov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KostyaEsmukov left a comment

Choose a reason for hiding this comment

Uh oh!

KostyaEsmukov commented May 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants