Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 15 additions & 13 deletions editoast/osm_to_railjson/src/osm_to_railjson.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,19 +68,8 @@ pub fn parse_osm(
adjacencies.entry(edge.target).or_default().edges.push(edge);
}

let nodes_tracks = NodeToTrack::from_edges(&edges);
let signals = signals(&osm_pbf_in, &nodes_tracks, &adjacencies);
let mut railjson = RailJson {
extended_switch_types: vec![],
detectors: signals.iter().map(detector).collect(),
signals,
speed_sections: rail_edges.clone().flat_map(speed_sections).collect(),
electrifications: rail_edges.clone().flat_map(electrifications).collect(),
operational_points: operational_points(&osm_pbf_in, &nodes_tracks),
..Default::default()
};

railjson.track_sections = rail_edges
let track_sections: Vec<TrackSection> = rail_edges
.clone()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
.clone()
.iter()

.map(|e| {
let geo = geos::geojson::Geometry::new(geos::geojson::Value::LineString(
e.geometry.iter().map(|c| vec![c.x, c.y]).collect(),
Expand Down Expand Up @@ -114,6 +103,19 @@ pub fn parse_osm(
})
.collect();

let nodes_tracks = NodeToTrack::from_edges(&edges);
let signals = signals(&osm_pbf_in, &nodes_tracks, &adjacencies);
let mut railjson = RailJson {
extended_switch_types: vec![],
detectors: signals.iter().map(detector).collect(),
signals,
speed_sections: rail_edges.clone().flat_map(speed_sections).collect(),
electrifications: rail_edges.clone().flat_map(electrifications).collect(),
Comment on lines +112 to +113
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
speed_sections: rail_edges.clone().flat_map(speed_sections).collect(),
electrifications: rail_edges.clone().flat_map(electrifications).collect(),
speed_sections: rail_edges.iter().copied().flat_map(speed_sections).collect(),
electrifications: rail_edges.iter().cpoied().flat_map(electrifications).collect(),

operational_points: operational_points(&osm_pbf_in, &nodes_tracks, &track_sections),
track_sections,
..Default::default()
};

for (node, mut adj) in adjacencies {
for e1 in &adj.edges {
for e2 in &adj.edges {
Expand Down
50 changes: 49 additions & 1 deletion editoast/osm_to_railjson/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,11 @@ use schemas::infra::Speed;
use schemas::infra::SpeedSection;
use schemas::infra::Switch;
use schemas::infra::TrackEndpoint;
use schemas::infra::TrackSection;
use schemas::primitives::Identifier;
use schemas::primitives::NonBlankString;
use std::collections::HashMap;
use std::collections::HashSet;
use std::str::FromStr;
use std::sync::atomic::AtomicU32;
use std::sync::atomic::Ordering;
Expand Down Expand Up @@ -461,9 +463,32 @@ fn map_node_id_to_node(
.collect()
}

/// Generate unique `local_track_names` for each `operational_point_parts`.
fn generate_local_track_name(parts: &mut [OperationalPointPart]) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would say that modifying the parameter is generally a bad pactise, and we should try to avoid it

I would prefer returning a new object. If we want to be memory conscious, we can move parts:

fn generate_local_track_name(parts: Vec<OperationalPointPart>) { // no & and then use into_iter

let mut used_name: HashSet<NonBlankString> = parts
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can’t we have HashSet<&NonBlankString> to avoid re-allocating the strings?

.iter()
.map(|part| part.local_track_name.clone())
.collect();
parts
.iter_mut()
.filter(|part| { part.local_track_name == "unknown".into()})
.for_each(|part| {
let mut track_number = 1;
part.local_track_name = loop {
let name = NonBlankString::from(track_number.to_string());
if !used_name.contains(&name) {
used_name.insert(name.clone());
break name;
}
track_number += 1;
};
});
}

pub fn operational_points(
osm_pbf_in: &std::path::PathBuf,
nodes_to_tracks: &NodeToTrack,
track_sections: &[TrackSection],
) -> Vec<OperationalPoint> {
let file = std::fs::File::open(osm_pbf_in).unwrap();
let mut pbf: OsmPbfReader<std::fs::File> = osm4routing::osmpbfreader::OsmPbfReader::new(file);
Expand All @@ -477,7 +502,7 @@ pub fn operational_points(
_ => None, // Discard Nodes and Ways
})
.flat_map(|rel| {
let parts: Vec<_> = rel
let mut parts: Vec<_> = rel
.refs
.iter()
.filter(|r| r.role == "stop") // We ignore other members of the relation
Expand All @@ -492,8 +517,31 @@ pub fn operational_points(
nodes_to_tracks
.track_and_position(node)
.map(|(track, position, local_track_name)| OperationalPointPart { track, position, local_track_name, extensions: Default::default() })
.map(|mut opp| {
Comment on lines 519 to +520
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should be able to merge those two .map(), feels weird to build the object first, then change one of its fields afterwards. Or at least invert the two .map() (first address the improvement of the local track name, then build the object).

// If the local_track_name is unknown, we try to find if the track_section associated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if we could extract this code and generate_local_track_name into a separate function

local_track_name_fallback(opp: &OperationalPointPart, track_sections: &[TrackSection]) -> NonBlankString

This would make it a bit more obvious that we have a double level of fallback and it will unnest

// to this operational_point_part has a track_name.
// If no track_name is found, keep it unknown and generate one in generate_local_track_name.
if opp.local_track_name == "unknown".into() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not tested, but I don’t think that you need to re-alloacate a string here

if &opp.local_track_name == "unknown" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You might be able to change the function track_and_position so it returns a Option<NonBlankString> instead of "unknown" when there is no local track name. This test here will then look a bit better.

opp.local_track_name = track_sections
.iter()
.find(|track_section| { track_section.id == opp.track })
.map(|track_section| {
if let Some(sncf) = &track_section.extensions.sncf
&& sncf.track_name != "??".into() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
&& sncf.track_name != "??".into() {
&& &sncf.track_name != "??" {

Some(sncf.track_name.clone())
}
else {
None
}
})
.expect("OperationalPointPart is linked to an invalid TrackSection id.")
Comment on lines +528 to +537
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I’m not sure, but can’t we replace the .map().expect() with a .and_then()? It’s not strictly equivalent I believe since you won’t panic when an operational point part is linked to an invalid track section, but it feels like you probably don’t want to interrupt the process in such case anyway (maybe log it at most?).

.unwrap_or(NonBlankString::from("unknown"));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not handling the fallback name here directly? I can understand that you are trying to generate names that don’t exist already, but the problem I see is that the current code rely on 2 different pieces of code that are in different places: here where you set up unknown, and in the other function, where you expect unknown. If some future developer, for whatever reason needs to change one or the other, it might start to not work anymore, and most likely, it will fail silently. It’s usually interesting to keep some locality in the decision and not spread it in different places (not always possible 🤷).

I’m under the impression that we basically want a random number, how about using UUID? This would remove entirely the need for the other function.

Another question is, why do we want to generate different names. From what I understand, the code before was generating unknown everywhere, can’t we keep that behavior?

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 initial idea for the name generation come from here, and the issue explain a bit more why it came to this.

I completly agree with the locality argument and I would love to use a UUID or just the same name for every track name. This would make my work so much easier.
But I think this would defeat the whole purpose of this PR (and maybe it should).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don’t think it defeats the purpose of the PR (not entirely at least), there is at least the part that try to look in extensions to grab some more track names. I posted a more detailed comment in the issue. Let’s see where that goes.

}
opp
})
})
.collect();
generate_local_track_name(&mut parts);
// Get operational point trigram
// Look through the nodes member of the relation and find one that has a "railway:ref" tag
let trigram = rel
Expand Down
Loading