-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FEAT] Changed id type to Union{Int, String} #101
[FEAT] Changed id type to Union{Int, String} #101
Conversation
442dc16
to
5d4bfbf
Compare
src/graph_utilities.jl
Outdated
Maps node id to index. | ||
""" | ||
node_id_to_index(g::OSMGraph, x::String) = g.node_to_index[x] | ||
node_id_to_index(g::OSMGraph, x::Vector{<:String}) = [node_id_to_index(g, i) for i in x] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't need this if x
is typed parametrically
src/graph_utilities.jl
Outdated
|
||
Maps node index to dijkstra state (parents). | ||
""" | ||
index_to_dijkstra_state(g::OSMGraph, x::String) = g.dijkstra_states[x] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
src/graph_utilities.jl
Outdated
|
||
""" | ||
node_id_to_dijkstra_state(g::OSMGraph, x::Integer) | ||
|
||
Maps node id to dijkstra state (parents). | ||
""" | ||
node_id_to_dijkstra_state(g::OSMGraph, x::Integer) = g.dijkstra_states[node_id_to_index(g, x)] | ||
""" | ||
node_id_to_dijkstra_state(g::OSMGraph, x::String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #101 +/- ##
==========================================
+ Coverage 80.03% 81.05% +1.02%
==========================================
Files 15 15
Lines 1142 1151 +9
==========================================
+ Hits 914 933 +19
+ Misses 228 218 -10 ☔ View full report in Codecov by Sentry. |
98e9dbc
to
b9408a2
Compare
b9408a2
to
18e73d4
Compare
src/parse.jl
Outdated
@@ -342,7 +343,9 @@ function init_graph_from_object(osm_xml_object::XMLDocument, | |||
filter_network_type::Bool=true | |||
)::OSMGraph | |||
dict_to_parse = osm_dict_from_xml(osm_xml_object) | |||
id_type = typeof(dict_to_parse["node"][1]["id"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment in parse_osm_network_dict
function
src/parse.jl
Outdated
@@ -357,7 +360,9 @@ function init_graph_from_object(osm_json_object::AbstractDict, | |||
filter_network_type::Bool=true | |||
)::OSMGraph | |||
dict_to_parse = osm_dict_from_json(osm_json_object) | |||
id_type = typeof(dict_to_parse["node"][1]["id"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment in parse_osm_network_dict
function
src/parse.jl
Outdated
network_type::Symbol=:drive; | ||
filter_network_type::Bool=true | ||
)::OSMGraph | ||
U = DEFAULT_OSM_INDEX_TYPE | ||
T = DEFAULT_OSM_ID_TYPE | ||
T = id_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do the check here so you don't need the id_type as a new input arg
T = id_type | |
T = get_id_type(osm_network_dict) |
where
function get_id_type(osm_network_dict::AbstractDict)::Type
if isempty(dict_to_parse["node"])
return Int64
end
first_id = dict_to_parse["node"][1]["id"]
if first_id isa Integer
return Int64
elseif first_id isa String
return String
else
throw(ErrorException("OSM ID type not supported: $(typeof(first_id))"))
end
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also write a doc string describing what is happening and the assumptions we're making:
- if the node set is empty we assume IDs are Int32
- if first node id is of an Integer type we also assume Int32
- if first node id is a String then we assume String
Hybrid types are not supported
a562b27
to
221e90b
Compare
3bba159
to
fd12a51
Compare
Lgtm |
c30254c
into
DeloitteOptimalReality:master
Added functionality to handle String ids.