Add geohashing capabilities #20

emirb · 2017-10-10T20:54:23Z

I'd like to propose two new functionalities to be added to geospatial extension:

geohash_encode
geohash_decode

Geohash is a geocoding system invented by Gustavo Niemeyer. https://en.wikipedia.org/wiki/Geohash

There is a significant performance gain from using this extension compared to user-land libraries.

Source code is available at https://github.com/emirb/php-geohash-ext
Rather than creating a standalone extension for the sole purpose of dealing with geohashes, a mutual agreement has been met concerning merging it to the existing geospatial PHP extension.

Add support for geohash_encode and geohash_encode Bump extension version to 0.1.2

codecov-io · 2017-10-10T21:01:34Z

Codecov Report

Merging #20 into master will increase coverage by 4.42%.
The diff coverage is 97.46%.

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
+ Coverage   63.09%   67.51%   +4.42%     
==========================================
  Files           2        3       +1     
  Lines         439      508      +69     
  Branches       67        0      -67     
==========================================
+ Hits          277      343      +66     
- Misses        162      165       +3

Impacted Files	Coverage Δ
geo_array.c	`56.52% <ø> (ø)`	⬆️
geohash.c	`100% <100%> (ø)`
geospatial.c	`64.2% <92.59%> (+0.74%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c03c43a...ba04711. Read the comment docs.

derickr

Hi!

Thanks for this PR, I like having this functionality. Also apologies for the large amounts of comments in this code review, but I feel these need to be addressed before I can merge this.

cheers,
Derick

derickr · 2017-10-11T10:38:42Z

geohash.h

+  | Authors: Derick Rethans <github@derickrethans.nl>                    |
+  |          Michael Maclean <michael@no-surprises.co.uk>                |
+  |          Nathaniel McHugh <nmchugh@inviqa.com>                       |
+  |          Marcus Deglos <marcus@deglos.com>                           |


These first four lines are not needed, as we didn't write any of this code.

derickr · 2017-10-11T10:38:51Z

geohash.h

+  +----------------------------------------------------------------------+
+  | PHP Version 5/7                                                      |
+  +----------------------------------------------------------------------+
+  | Copyright (c) 1997-2016 The PHP Group                                |


It should only be "2017" in here.

derickr · 2017-10-11T10:40:33Z

geohash.h

+  |          Emir Beganovic <emir@php.net>                               |
+  +----------------------------------------------------------------------+
+*/
+


Header files should have a guard, such as done at:

https://github.com/php-geospatial/geospatial/blob/master/php_geospatial.h#L22

I just realized geo_array.h doesn't have that either, so created a ticket: #21

derickr · 2017-10-11T10:43:02Z

geohash.h

+typedef struct IntervalStruct {
+    double high;
+    double low;
+} Interval;


The current coding standard would have used "interval_struct", but it looks like this is something internal to the geohash algorithm, so should probably not be in a public header.

derickr · 2017-10-11T10:43:21Z

geohash.h

+typedef struct GeoCoordStruct {
+    double latitude;
+    double longitude;
+} GeoCoord;


Ditto here, these should both be geo_coord - and private.

derickr · 2017-10-11T10:54:05Z

geospatial.c

+
+    zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "dd|l", &lat, &lng, &precision);
+    char* hash;
+    hash = _geohash_encode(lat, lng, precision);


new line between declarations and code please, and none needed among the declarations.

derickr · 2017-10-11T10:54:25Z

geospatial.c

+#endif
+}
+
+/* {{{ string geohash_decode( [ string $geohash ] )


$geohash is not optional here, so shouldn't have [ .. ]

derickr · 2017-10-11T10:54:45Z

geospatial.c

+
+    array_init(return_value);
+    add_assoc_double(return_value, "latitude", area.latitude);
+    add_assoc_double(return_value, "longitude", area.longitude);


The return of this should be a GeoJSONPoint.

derickr · 2017-10-11T10:55:24Z

php_geospatial.h

@@ -22,7 +22,7 @@
 #ifndef PHP_GEOSPATIAL_H
 #define PHP_GEOSPATIAL_H

-#define PHP_GEOSPATIAL_VERSION "0.1.1-dev"
+#define PHP_GEOSPATIAL_VERSION "0.1.2"


No need to bump the version, this we only do when making a release. However, you probably should change it to 0.2.0-dev, as there is new functionality with this PR.

derickr · 2017-10-11T10:56:40Z

php_geospatial.h

@@ -143,6 +150,8 @@ PHP_FUNCTION(vincenty);
 PHP_FUNCTION(rdp_simplify);
 PHP_FUNCTION(interpolate_linestring);
 PHP_FUNCTION(interpolate_polygon);
+PHP_FUNCTION(geohash_encode);
+PHP_FUNCTION(geohash_decode);


Please also add documentation and an example to the README.rst file.

derickr · 2017-10-23T19:10:18Z

README.rst

+        [0]=>
+        float(16.4)
+        [1]=>
+        float(48.2)


I get the following output:

array(2) { 'type' => string(5) "Point" 'coordinates' => array(2) { [0] => double(171.02859674022) [1] => double(49.674154138193) } }

Perhaps make the input the same as the output from above?

I've adjusted it but bear in mind that the output of a 12 char geohash will be a a float with 12 decimal places because of high detail level.

derickr · 2017-10-23T19:11:02Z

README.rst

+
+The `geohash_encode` function can be used to convert GeoJSON Point to a geohash::
+
+    $geohash = geohash_encode(array('type' => 'Point', 'coordinates' => [16.4, 48.2]), 12);


I would change this to:

echo geohash_encode(array('type' => 'Point', 'coordinates' => [16.4, 48.2]), 12);

As that actually outputs, and perhaps explain the 12 as well - which I presume is the length of the returned string?

derickr · 2017-10-23T19:11:43Z

geo_lat_long.h

+   double   height;
+} geo_lat_long;
+
+#endif


Nit pick: misses CR at end of line.

derickr · 2017-10-23T19:12:37Z

geohash.c

+typedef struct IntervalStruct {
+    double high;
+    double low;
+} interval_struct;


I think you missed changing "struct IntervalStruct" to "struct interval_string" here? It's not used really, so the compiler wouldn't bitch.

derickr · 2017-10-23T19:13:25Z

geohash.c

+    hash = (char*)safe_emalloc(precision, sizeof(char), 1);
+
+    hash[precision] = '\0';
+    steps = precision*5.0;


nit pick: add spaces around the *.

derickr · 2017-10-23T19:22:32Z

geo_lat_long.h

+
+typedef struct {
+   double   latitude;
+   double   longitude;


I realise this was my mistake, but can you swap latitude and longitude, so that the order is x, y, and z ?

derickr · 2017-10-23T19:22:50Z

geo_lat_long.h

+  +----------------------------------------------------------------------+
+  | PHP Version 7                                                      |
+  +----------------------------------------------------------------------+
+  | Copyright (c) 1997-2016 The PHP Group                                |


You can just use "2017" here.

derickr · 2017-10-23T19:24:10Z

geospatial.c

+		RETURN_FALSE;
+	}
+
+    char* hash;


This needs to be up with the other variable declarations, as in C you can't mix code and declarations.

derickr · 2017-10-23T19:25:02Z

geospatial.c

+        return;
+    }
+
+    geo_lat_long area = php_geo_geohash_decode(hash);


Can the decoding altogether fail? With an invalid string for example?

Not really, I've tried to break it though with utf8 chars and so on...

derickr · 2017-10-23T19:25:57Z

tests/geohash_encode.phpt

+echo geohash_encode(array('type' => 'Point', 'coordinates' => [90, 90]), 6).PHP_EOL;
+echo geohash_encode(array('type' => 'Point', 'coordinates' => [16.363, 48.21]), 32).PHP_EOL;
+echo geohash_encode(array('type' => 'Point', 'coordinates' => [95, 95]), 6).PHP_EOL;
+echo geohash_encode(array('type' => 'Point', 'coordinates' => [185, 185]), 12).PHP_EOL;


Are these numbers in the last two not out of range? Shouldn't that make it return false?

By definition of the geohashing algorithm there should always be a return AFAIK, and or everything out of bounds >180 it will return zzzzzzzzzzzz. This is the behaviour that I've seen in geohash libraries in other languages as well.

derickr · 2017-11-17T12:52:00Z

Hi!

Are you still interested in working further on this?

cheers,
Derick

emirb · 2017-11-28T11:34:28Z

Hi @derickr.

I didn't have time lately to finish final changes on this but will do so today or tomorrow.

Thanks for your patience!

Cheers

emirb · 2017-12-24T15:47:49Z

@derickr Sorry for the long delay. I hope that's it now and that you can finally merge the code additions and release new version.

Merry Christmas!

derickr · 2017-12-29T12:12:15Z

Merged, after rebase, and some code standard changes. I've also released 0.2.0.

Emir Beganovic added 4 commits October 10, 2017 22:44

Add geohashing capabilities

d5c5294

Add support for geohash_encode and geohash_encode Bump extension version to 0.1.2

use fabs for floating point value

039079e

Code style

4b473c5

Use PHP_VERSION_ID instead of PHP_MAJOR_VERSION

f4a149d

derickr requested changes Oct 11, 2017

View reviewed changes

Emir Beganovic added 11 commits October 22, 2017 21:53

Code style

60914e9

Add header guard

3d285cd

Create a separate geohash.c file

c002f80

Update tests

907af0f

Reuse geo_lat_long definition

3ffedeb

Update readme

40f866b

Leave out version bump for now

a12fb04

Move char_map to geohash.c

e2d5a32

Debug

dc21c23

Whitespaces cleanup

98822e9

Do not test for unicode geohash_decode as it breaks

e48862f

derickr reviewed Oct 25, 2017

View reviewed changes

Emir Beganovic added 6 commits December 24, 2017 16:29

Code style fixes

e8b6e27

Missing CR

5f2b747

Small rename

0e26b34

Move constants from header file

e7699ba

Missing CR

2b930e9

Rename struct members

ba04711

derickr added a commit that referenced this pull request Dec 29, 2017

Merged pull request #20

fa1844b

derickr closed this Dec 29, 2017

emirb deleted the feature/geohash branch December 29, 2017 13:11


		The `geohash_encode` function can be used to convert GeoJSON Point to a geohash::

		$geohash = geohash_encode(array('type' => 'Point', 'coordinates' => [16.4, 48.2]), 12);

Add geohashing capabilities #20

Add geohashing capabilities #20

Uh oh!

Conversation

emirb commented Oct 10, 2017

Uh oh!

codecov-io commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

derickr 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

derickr commented Nov 17, 2017

Uh oh!

emirb commented Nov 28, 2017

Uh oh!

emirb commented Dec 24, 2017

Uh oh!

derickr commented Dec 29, 2017

Uh oh!

Uh oh!

codecov-io commented Oct 10, 2017 •

edited

Loading