-
Notifications
You must be signed in to change notification settings - Fork 89
core, editoast: add topological to geometric offset projection on /path endpoint #17476
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,7 @@ import fr.sncf.osrd.api.RangeValues | |||||||||
| import fr.sncf.osrd.path.interfaces.PhysicsPath | ||||||||||
| import fr.sncf.osrd.railjson.schema.geom.RJSLineString | ||||||||||
| import fr.sncf.osrd.utils.json.UnitAdapterFactory | ||||||||||
| import fr.sncf.osrd.utils.units.Distance | ||||||||||
| import fr.sncf.osrd.utils.units.Offset | ||||||||||
|
|
||||||||||
| class PathPropResponse( | ||||||||||
|
|
@@ -18,6 +19,7 @@ class PathPropResponse( | |||||||||
| val geometry: RJSLineString, | ||||||||||
| @Json(name = "operational_points") val operationalPoints: List<OperationalPointResponse>, | ||||||||||
| val zones: RangeValues<String>, | ||||||||||
| @Json(name = "geom_projection") val geomProjection: GeometricProjection, | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| interface Electrification | ||||||||||
|
|
@@ -54,6 +56,22 @@ data class OperationalPointPartExtension(val sncf: OperationalPointPartSncfExten | |||||||||
|
|
||||||||||
| data class OperationalPointPartSncfExtension(val kp: String) | ||||||||||
|
|
||||||||||
| data class GeometricProjection( | ||||||||||
| @Json(name = "topo_offsets") val topoOffsets: List<Offset<PhysicsPath>>, | ||||||||||
| @Json(name = "geom_offsets") val geomOffsets: List<Offset<RJSLineString>>, | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's about to become RJSMutliLineString 😅 I'd say the first one merged wins (but notify the other to propagate the change)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for this, I’ll be watching if that PR is merged before this one |
||||||||||
| ) { | ||||||||||
|
bougue-pe marked this conversation as resolved.
|
||||||||||
| init { | ||||||||||
| // There must be the same number of topological boundaries and geometric boundaries | ||||||||||
| // and at least two of each (the beginning and the end) | ||||||||||
| assert(topoOffsets.size == geomOffsets.size && topoOffsets.size >= 2) | ||||||||||
| // Each list must start by 0 | ||||||||||
| assert(topoOffsets[0].distance == Distance.ZERO && geomOffsets[0].distance == Distance.ZERO) | ||||||||||
| // Each list must be increasing (not strictly) | ||||||||||
| topoOffsets.zipWithNext().all { it.first <= it.second } | ||||||||||
| geomOffsets.zipWithNext().all { it.first <= it.second } | ||||||||||
|
Comment on lines
+70
to
+71
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is missing the assert:
Suggested change
|
||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| val polymorphicElectrificationAdapter: PolymorphicJsonAdapterFactory<Electrification> = | ||||||||||
| PolymorphicJsonAdapterFactory.of(Electrification::class.java, "type") | ||||||||||
| .withSubtype(Electrified::class.java, "electrification") | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,6 +6,7 @@ import fr.sncf.osrd.api.RangeValues | |||||||||||||
| import fr.sncf.osrd.api.path_properties.* | ||||||||||||||
| import fr.sncf.osrd.cli.RqFake | ||||||||||||||
| import fr.sncf.osrd.railjson.schema.common.graph.EdgeDirection | ||||||||||||||
| import fr.sncf.osrd.utils.units.Distance | ||||||||||||||
| import fr.sncf.osrd.utils.units.Offset | ||||||||||||||
| import fr.sncf.osrd.utils.units.meters | ||||||||||||||
| import kotlin.test.assertEquals | ||||||||||||||
|
|
@@ -86,6 +87,25 @@ class PathPropEndpointTest : ApiTest() { | |||||||||||||
| ), | ||||||||||||||
| ) | ||||||||||||||
| assertEquals(parsed.operationalPoints, oPs) | ||||||||||||||
| // Check topological distance to geometric distance projection | ||||||||||||||
| // The repetition of the last two values is because of a null-length range | ||||||||||||||
| // on the TA3 track section | ||||||||||||||
| val geomProjection = | ||||||||||||||
| GeometricProjection( | ||||||||||||||
| listOf( | ||||||||||||||
| Offset.zero(), | ||||||||||||||
| Offset(1_950.meters), | ||||||||||||||
| Offset(3_900.meters), | ||||||||||||||
| Offset(3_900.meters), | ||||||||||||||
| ), | ||||||||||||||
| listOf( | ||||||||||||||
| Offset.zero(), | ||||||||||||||
| Offset(Distance(2464352)), | ||||||||||||||
| Offset(Distance(4630820)), | ||||||||||||||
| Offset(Distance(4630820)), | ||||||||||||||
|
Comment on lines
+103
to
+105
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's probably not worth the previous geo distance processing, but naming the values would be easier to read/review (in case value changes, the intent is clearer):
Suggested change
|
||||||||||||||
| ), | ||||||||||||||
| ) | ||||||||||||||
| assertEquals(geomProjection, parsed.geomProjection) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| @Test | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -35,6 +35,8 @@ pub struct PathPropertiesResponse { | |||||
| pub operational_points: Vec<OperationalPointOnPath>, | ||||||
| /// Zones along the path | ||||||
| pub zones: PropertyZoneValues, | ||||||
| // Projection from topologic offset to geometric offset | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Last one if I'm correct:
Suggested change
|
||||||
| pub geom_projection: GeometryProjection, | ||||||
| } | ||||||
|
|
||||||
| /// Property f64 values along a path. Each value is associated to a range of the path. | ||||||
|
|
@@ -162,6 +164,32 @@ impl PropertyZoneValues { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Projection to map topological offset to geometric offset (or reversed). | ||||||
| /// topo_offsets and geom_offsets are the same size | ||||||
| #[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] | ||||||
| #[schema(as = CorePropertyGeometryProjection)] | ||||||
| pub struct GeometryProjection { | ||||||
|
woshilapin marked this conversation as resolved.
|
||||||
| /// Topological offsets in millimeters. | ||||||
| /// Starts with 0 and is increasing. | ||||||
| #[schema(min_items = 2)] | ||||||
| topo_offsets: Vec<u64>, | ||||||
| /// Geometric offsets in millimeters. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
(this should be propagated to openapi, which is the most important to document) |
||||||
| /// Starts with 0 and is increasing. | ||||||
| #[schema(min_items = 2)] | ||||||
| geom_offsets: Vec<u64>, | ||||||
| } | ||||||
|
woshilapin marked this conversation as resolved.
|
||||||
|
|
||||||
| impl GeometryProjection { | ||||||
| pub fn new(topo_offsets: Vec<u64>, geom_offsets: Vec<u64>) -> Self { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to create a fallible constructor instead: But from what I see in the code, this constructor only exists for tests purpose. Maybe the function can also be annotated with
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Going for |
||||||
| assert_eq!(topo_offsets.len(), geom_offsets.len()); | ||||||
| assert!(topo_offsets.len() >= 2); | ||||||
| Self { | ||||||
| topo_offsets, | ||||||
| geom_offsets, | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| impl AsCoreRequest<Json<PathPropertiesResponse>> for PathPropertiesRequest<'_> { | ||||||
| const URL_PATH: &'static str = "/path_properties"; | ||||||
|
|
||||||
|
|
||||||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Please comment here that Haversine formula is used.