OSM Doc + route length minor improvements

[Datseris]

• Small improvements to docstrings in OSM
• Closes [FR] Obtain total route length in OSM #628 (Currently WIP + needs tests)
• Updates to LightOSM 0.2
• Closes CompatHelper: bump compat for LightOSM to 0.2, (keep existing compat) #631

[codecov-commenter]

Codecov Report

Merging #629 (68f6429) into master (dbddf76) will decrease coverage by 0.49%.
The diff coverage is 58.06%.

❗ Current head 68f6429 differs from pull request most recent head 313765c. Consider uploading reports for the commit 313765c to get more accurate results

@@            Coverage Diff             @@
##           master     #629      +/-   ##
==========================================
- Coverage   91.04%   90.55%   -0.50%     
==========================================
  Files          24       24              
  Lines        1631     1641      +10     
==========================================
+ Hits         1485     1486       +1     
- Misses        146      155       +9     

Impacted Files	Coverage Δ
src/spaces/openstreetmap.jl	71.68% <58.06%> (-1.88%)	⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[Datseris]

@AayushSabharwal its obviously wrong the way I've coded route_length, because looking at the test suite there is some reverse that needs to be done. In the test suite I see that the length of a route is supposed to be:

    len = sum(LightOSM.weights_from_path(
        model.space.map,
        reverse([model.space.map.index_to_node[i] for i in model.space.routes[1].route]),
    ))

We need to better document the struct OpenStreetMapPath because all of these information are not really there. I didn't know that one has to (1) transform the indices and (2) to reverse the sequence. Can you give some more background please, and I'll go ahead and add the documentation.

[AayushSabharwal]

Sorry for the delay, I've been really busy

Can you give some more background please, and I'll go ahead and add the documentation.

Sure. Each node has a node ID from the OpenStreetMap. The map is stored as a Graph by LightOSM, and hence each node also has a vertex index corresponding to the vertex representing it in this graph. Hence each node can be referred to by either the node ID or its graph index, and we can convert either way. OSMSpace uses graph vertex indices consistently, since we access graph data more often than OSM data. The index_to_node lookup serves to convert each vertex index to its node ID from OSM.

LightOSM.weights_from_path expects a Vector of node IDs along the path as its second parameter, in the order they are visited. The OpenStreetMapPath struct stores them in reverse order, so a vertex can be popped off the end once it is reached. As a result, the vector needs to be reversed.

LightOSM.weights_from_path returns a vector of the corresponding edge weights along the path, which is summed to get the length of the route.

[Datseris]

Sorry for the delay, I've been really busy

No worries, open source = very low response rate is acceptable!

Each node has a node ID from the OpenStreetMap. The map is stored as a Graph by LightOSM, and hence each node also has a vertex index corresponding to the vertex representing it in this graph

OOF, is there a reason these two things aren't the same thing? I don't see a reason, given that both are the integers from 1 to the total number of nodes...

The OpenStreetMapPath struct stores them in reverse order, so a vertex can be popped off the end once it is reached

Okay, but why do we do that this way, instead of storing them in the order they are visited and using popfirst! instead of pop! ...?

EDIT: Also, to clarify: the source code comment said that the field stores the indices from start to destianation, which is not reverse order. So, to be sure, what is the order that the indices are currently stored in the struct? The one you told me here, or the one that was in the original comment in the source code (which said from start to destination)?

---

I'll utilize the docstring of OpenStreetMapPath to make it something like developer's docs for OSM.

[Datseris]

Aaaaah the "OpenStreetMap ID" is like the global ID for the whole globe I guess, which is why the vertex node index is different.

[AayushSabharwal]

Aaaaah the "OpenStreetMap ID" is like the global ID for the whole globe I guess, which is why the vertex node index is different.

Yeah. They're defined by OpenStreetMap, and are same for the same node in two different maps. The vertex node index varies.

Okay, but why do we do that this way, instead of storing them in the order they are visited and using popfirst! instead of pop! ...?

popfirst! is O(n), and pop! is O(1)

So, to be sure, what is the order that the indices are currently stored in the struct? The one you told me here, or the one that was in the original comment in the source code (which said from start to destination)?

What I've said here. I think why I wrote it that way in the comment is because the path is calculated from start to dest, but stored in reverse order. This would (in the general case) be different from the path from dest to start, partially because of edge cases, and mainly because some roads might be one-way. The comment should've been more explicit about this.

[Datseris]

In this PR let's also try to update LightOSM to 0.2, https://github.com/DeloitteDigitalAPAC/LightOSM.jl/releases/tag/v0.2.0 . At the moment I'm busy with another project at DynamicalSystems.jl, but i'll try to get around doing this here when possible. Feel free to add commits here if you have time, but no stress.

popfirst! is O(n), and pop! is O(1)

Where did you find this information?

[fbanning]

popfirst! is O(n), and pop! is O(1)

Where did you find this information?

I think the statement is generally correct from a theoretical viewpoint on operation complexity because of the required memory movement when using popfirst. But I'm not so sure if using it is really slower because it is likely a highly optimised operation by the compiler.

I was intrigued to try this out and quick (admittedly crude) tests confirmed @AayushSabharwal 's approach to be the faster one:

# small array
julia> @btime (a = collect(1:100); for _ in 1:100; popfirst!(a); end)
  563.196 ns (1 allocation: 896 bytes)

julia> @btime (a = collect(1:100); reverse!(a); for _ in 1:100; pop!(a); end)
  416.166 ns (1 allocation: 896 bytes)

# bigger array
julia> @btime (a = collect(1:1_000_000); for _ in 1:1_000_000; popfirst!(a); end)
  6.016 ms (2 allocations: 7.63 MiB)

julia> @btime (a = collect(1:1_000_000); reverse!(a); for _ in 1:1_000_000; pop!(a); end)
   4.987 ms (2 allocations: 7.63 MiB)

[AayushSabharwal]

This fixes the TODO we had with try...catch around shortest_path. It returns nothing now.

Also, LightOSM also exports AStar now, which conflicts with Pathfinding.AStar if both are imported

[Datseris]

thanks a lot for the help. now we just have to finish the docs and ensure tests are passable.

[Datseris]

@AayushSabharwal that is odd, it seems like that all failing tests are about the pathfinding (like, our own Astar, not the OSM stuff)

[AayushSabharwal]

The issues:

• Some @testsets didn't have ends
• Refactoring start_i to start_intersection, etc. didn't apply everywhere
• Variables are scoped to the @testset they're defined in, so ones like start_intersection that are used everywhere had to be defined outside
• LightOSM.AStar conflicted with Pathfinding.AStar. Turns out LightOSM is only imported for a single function LightOSM.weights_from_path which can instead be used as OSM.LightOSM.weights_from_path to avoid importing LightOSM.

[Datseris]

haha sorry you had to fix my tests breakages! I wanted to redesign the tests to follow more #634 , but then I got busy for a deadline for Thursday!!! I'll have a look at the documentation failing test and then merge!

[AayushSabharwal]

I'm happy to have helped :)

[Datseris]

Note to self: still needs test for the new function route_length.

[Datseris]

So docs fail because interactive dynamics needs to be updated to latest Makie 0.17, which I hope to do this sunday...

[Datseris]

(Tests pass, they were passing before, and I just added another passing one. We haven't merged so far because docs weren't building because of some missmatch in versions)