Skip to content

Refactor Hilbert values computation#3354

Merged
TheMarex merged 2 commits intomasterfrom
refactor-hilbert-values
Nov 28, 2016
Merged

Refactor Hilbert values computation#3354
TheMarex merged 2 commits intomasterfrom
refactor-hilbert-values

Conversation

@oxidase
Copy link
Copy Markdown
Contributor

@oxidase oxidase commented Nov 22, 2016

Issue

The current Hilbert codes is an SFC but not the original Hilbert curve #3352. The PR fixes Hilbert code computation. There are 6 failing test cases, that failing due to reordering of edges in the R-tree. For example, features/testbot/compression.feature:7 returns phantom node that uses 3 segments instead of 4, so the small adjustment of 0.11 meters is not added. Other tie-breaking cases look not so trivial and must be clarified first.

Tasklist

  • refactor implementation of Hilbert linear codes
  • add regression / cucumber cases (see docs/testing.md)
  • clarify failing features/car/maxspeed.feature:81
  • clarify failing features/car/maxspeed.feature:100
  • clarify failing features/car/summaries.feature:9
  • clarify failing features/car/traffic_turn_penalties.feature:105
  • clarify failing features/testbot/bearing_param.feature:48
  • review
  • adjust for comments

Requirements / Relations

Arndt, Jörg. Matters Computational Ideas, Algorithms, Source Code, 2010. p86
http://home.pipeline.com/~hbaker1/hakmem/topology.html#item115

@danpat
Copy link
Copy Markdown
Member

danpat commented Nov 22, 2016

Quick comparison using rtree-bench with a small dataset (monaco-latest.osm):

Before this change:

Running raw RTree queries (1 result) with 10000 coordinates: Took 14.0459 seconds (14045.9ms)  ->  1.40459 ms/query (14045.9ms)
Running raw RTree queries (10 results) with 10000 coordinates: Took 14.3965 seconds (14396.5ms)  ->  1.43965 ms/query (14396.5ms)

After:

Running raw RTree queries (1 result) with 10000 coordinates: Took 13.4529 seconds (13452.9ms)  ->  1.34529 ms/query (13452.9ms)
Running raw RTree queries (10 results) with 10000 coordinates: Took 13.6612 seconds (13661.2ms)  ->  1.36612 ms/query (13661.2ms)

Looks like the corrected curve shape could give us a modest performance boost due to improved cache locality.

@oxidase
Copy link
Copy Markdown
Contributor Author

oxidase commented Nov 27, 2016

Tests features/testbot/compression.feature:7, features/car/maxspeed.feature:81, features/car/maxspeed.feature:100 use exact values for expectations, but other ordering could return different edges for phantom nodes and due to some round-off errors introduce 0.11 meters segments that correspond to 0.000001 coordinate precision. Expectation in tests are update to the new values that still depend on an edges ordering in R-tree.

related #3368

@oxidase
Copy link
Copy Markdown
Contributor Author

oxidase commented Nov 27, 2016

Tests features/car/summaries.feature:9, features/car/traffic_turn_penalties.feature:105 fail due to 0-length segments in snapped phantom nodes.
Must be fixed in #2287

@oxidase
Copy link
Copy Markdown
Contributor Author

oxidase commented Nov 27, 2016

Test features/testbot/bearing_param.feature:48 fail due to different components of the source and the target phantom nodes.

It should not be in real cases, so i would suggest to remove ha edge, but just commented out the test row

@oxidase oxidase force-pushed the refactor-hilbert-values branch 2 times, most recently from 9986141 to e3e83cd Compare November 27, 2016 17:37
# The target point `a` can belong to either `ab` or `ha` edge.
# The latter case leads to "Impossible route between points",
# because `ab` and `ha` are in different components
#| 1 | d | 90 90 | ab,bc,cd,da,da | 0->90,90->0,0->270,270->180,180->0 |
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this. We are not trying to snap to a, we are trying to snap to a coordinate close to 1 (which is just a coordinate not linked to an OSM node). It should be something in the middle of ab (which has bearing 90°).

The target segment for d should be unique as well. It is only contained by one segment (cd) with 90°. I'm quite confused why the original test specifies da as target segment which has bearing 180 and should not be chosen as segment for d.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is in the target point for d. Here the nearest edges to d:

input_coordinate = (lon:1.000089, lat:1)
   2,1 "cd" distance 0.111226299998 fwd bearing 270.000007845 invalid, bck bearing 90.0000078449 invalid
   1,5 "da" distance 0.111226299998 fwd bearing 180 invalid, bck bearing 360 invalid
   1,0 "dg" distance 0.111226299998 fwd bearing 270.000000777 invalid, bck bearing 90.0000007767 invalid
   4,5 "ha" distance 10.1215933 fwd bearing 89.9999992235 valid, bck bearing 269.999999223 invalid
   5,6 "ab" distance 10.1215933 fwd bearing 90 valid, bck bearing 270 invalid 0.1112263

With the new ordering ha is before ab and ha also has bearing 90, so it is a valid phantom node, but ha is in another scc. One possibility would be to introduce point 2 close to d on the edge cd and use instead of d

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah now I understand. Yes we should fix this test case by adding a middle node 2 on da. I think I misunderstood the original test, it seems like we want to snap to da because cd has bearing 270° (since it is going in the different direction).

@oxidase oxidase force-pushed the refactor-hilbert-values branch from e3e83cd to b925e3d Compare November 28, 2016 10:13
| from | to | bearings | route | bearing |
| 0 | b | 10 10 | bc,bc | 0->0,0->0 |
| 0 | b | 90 90 | ab,ab | 0->90,90->0 |
| 0 | b | 170 170 | da,da | 0->0,0->0 |
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have uncommented two checks here, both 0 and b project to a on da, so the result is a 0-size segment on da

Copy link
Copy Markdown
Member

@TheMarex TheMarex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good to me.

@TheMarex TheMarex merged commit e343f71 into master Nov 28, 2016
@TheMarex TheMarex deleted the refactor-hilbert-values branch November 28, 2016 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants