extract functions (Geojson to Topojson)#46
Conversation
|
@patricoferris what do i think about this? I also intend to add |
| loop [] coordinates | ||
| | _ -> failwith "Not a valid LineString or Polygon" | ||
|
|
||
| let extract_lines = function |
There was a problem hiding this comment.
This comment applies to pretty much most of the other functions defined here.
I don't think we want to reimplement all of the JSON parsing logic again, GeoJSON does this already for us so we should just uses GeoJSON to extract the coordinates etc. from the geometries (since we are converting from GeoJSON to TopoJSON).
There was a problem hiding this comment.
| val to_json : ?bbox:float array -> t -> json | ||
| val of_json : json -> (t, [ `Msg of string ]) result | ||
|
|
||
| val extract_lines : |
There was a problem hiding this comment.
We won't actually ever expose these internal functions. Instead, eventually, we will just have something like:
val geojson_to_topojson : Geojson.t -> Topojson.t
|
Hi @patricoferris, module Geojson = Geojson.Make (Ezjsonm_parser)keep giving error Also with the current implementation, will |
I think so, they are after all just lines.
Yep, we still want to be agnostic to the JSON parser the user decided to use. You should be able to pass the same one that gets passed to our module Make (J : Intf.Json) = struct
module Geojson = Geojson.Make(J)
... |
|
Hi @patricoferris what do you think? |
| let arr = Array.map LineString.coordinates lines in | ||
| Array.to_list arr | ||
| | Polygon p -> | ||
| let rings = Polygon.rings p in |
There was a problem hiding this comment.
Hmm thinking about this a little more, perhaps we have to treat rings slightly differently because the first and last point are technically connected.
| List.iter | ||
| (fun line -> | ||
| let line_length = Array.length line in | ||
| for i = 0 to line_length - 2 do |
There was a problem hiding this comment.
Points on a line other than the first and the last have two neighbours: n - 1 and n + 1.
| let find_junctions (lines : Geojson.Geometry.Position.t array list) : | ||
| Geojson.Geometry.Position.t list = | ||
| let open Geojson.Geometry in | ||
| let junction_table : (Position.t, Position.t list) Hashtbl.t = |
There was a problem hiding this comment.
Why not make this a position to position set hash table to stay closer to how the blog post does it?
| for i = 0 to line_length - 2 do | ||
| let point1 = line.(i) in | ||
| let point2 = line.(i + 1) in | ||
| if Hashtbl.mem junction_table point1 then |
There was a problem hiding this comment.
I think for every point we visit we want to put in the hash table it's neighbours as a set. So if we are at point N then we but N, { N - 1, N - 2} as a key-value pair into the hash table.
Then if we come across N again we check that Ns neighbours are different (therefore a junction) or if they are the same then it isn't.
|
@patricoferris I am having a lot of type incompatibility errors. Please help me out 😥 |
|
Ah, most of the issues were just related to giving functions a little too much generality and also we didn't expose that the positions were float arrays. Feel free to cherry-pick this commit 7b31814 |
|
The type errors are gone now. :)) Thank you @patricoferris |
|
Do you think the "coordinates": [
[
[1, 1], [2, 2],[3, 3],
[4, 4], [3, 1],[4, 2]
], [
[4, 2], [4, 4], [4, 6]
]
]And the test didn't print out any junctions |
|
I don't think the function is correct, see my comments inline on the code. |
patricoferris
left a comment
patricoferris
left a comment
There was a problem hiding this comment.
These changes are heading in the right direction, nice! Comments are inline.
| let transform = Alcotest.testable pp_transform Stdlib.( = ) | ||
|
|
||
| let to_string pp a = | ||
| let _to_string pp a = |
There was a problem hiding this comment.
If this function isn't being used we can probably remove it ?
| let point1 = line.(i) in | ||
| let point2 = line.(i - 1) in | ||
| let point3 = line.(i + 1) in | ||
| let point2 = if i > 0 then line.(i - 1) else line.(line_length - 1) in |
There was a problem hiding this comment.
Isn't this only true if we're talking about a ring rather than any line?
| let point2 = line.(i - 1) in | ||
| let point3 = line.(i + 1) in | ||
| let point2 = if i > 0 then line.(i - 1) else line.(line_length - 1) in | ||
| let point3 = if i < line_length - 1 then line.(i + 1) else line.(0) in |
| @@ -698,24 +696,23 @@ module Make (J : Intf.Json) = struct | |||
| let junction_table : (Position.t, Set.t) Hashtbl.t = | |||
There was a problem hiding this comment.
We really want to know every set of neighbours to begin with (let's get the algorithm correct before optimising). So I think we want something like: (Position.t, Set.t list) Hashtbl.t.
| let point3 = if i < line_length - 1 then line.(i + 1) else line.(0) in | ||
| if Hashtbl.mem junction_table point1 then | ||
| let neighbors = Hashtbl.find junction_table point1 in | ||
| Hashtbl.replace junction_table point1 |
There was a problem hiding this comment.
Instead of adding the neighbours to an ever growing set, I think we want to keep them distinct so we know what two neighbours belong together. Consider the problem of having two lines ABC and CBA. B in this case is not a junction because the neighbours A and C are the same in both lines.
| let neighbors = Hashtbl.find junction_table point1 in | ||
| if Set.cardinal neighbors > 2 then | ||
| ref junction := point1 :: !(ref junction) | ||
| if Set.cardinal neighbors > 2 then junction := point1 :: !junction |
There was a problem hiding this comment.
With the above changes this will become List.length when we store the list of possible neighbours and store a new pair iff they are different as defined in the algorithm.
2 participants
Extracts lines and rings from Geojson's Objects