Skip to content
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

Fix timestops margins #8077

Merged
merged 5 commits into from
Jul 18, 2024
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
"values": [
"5%",
"3min/100km",
"none"
"0%"
]
},
"initial_speed": 2.5,
Expand Down
37 changes: 13 additions & 24 deletions editoast/editoast_schemas/src/train_schedule/margins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ editoast_common::schemas! {
pub struct Margins {
#[schema(inline)]
pub boundaries: Vec<NonBlankString>,
#[derivative(Default(value = "vec![MarginValue::None]"))]
#[derivative(Default(value = "vec![MarginValue::default()]"))]
/// The values of the margins. Must contains one more element than the boundaries
/// Can be a percentage `X%`, a time in minutes per 100 kilometer `Xmin/100km` or `none`
#[schema(value_type = Vec<String>, example = json!(["none", "5%", "2min/100km"]))]
/// Can be a percentage `X%` or a time in minutes per 100 kilometer `Xmin/100km`
#[schema(value_type = Vec<String>, example = json!(["5%", "2min/100km"]))]
pub values: Vec<MarginValue>,
}

Expand All @@ -43,11 +43,10 @@ impl<'de> Deserialize<'de> for Margins {
}
}

#[derive(Debug, Copy, Clone, Default, PartialEq, Derivative)]
#[derivative(Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Derivative)]
#[derivative(Hash, Default)]
pub enum MarginValue {
#[default]
None,
#[derivative(Default)]
Percentage(#[derivative(Hash(hash_with = "editoast_common::hash_float::<3,_>"))] f64),
MinPer100Km(#[derivative(Hash(hash_with = "editoast_common::hash_float::<3,_>"))] f64),
}
Expand All @@ -58,20 +57,17 @@ impl<'de> Deserialize<'de> for MarginValue {
D: serde::Deserializer<'de>,
{
let value = String::deserialize(deserializer)?;
if value.to_lowercase() == "none" {
return Ok(Self::None);
}
if value.ends_with('%') {
let float_value = f64::from_str(value[0..value.len() - 1].trim()).map_err(|_| {
serde::de::Error::invalid_value(
serde::de::Unexpected::Str(&value),
&"a valid float",
)
})?;
if float_value <= 0.0 {
if float_value < 0.0 {
return Err(serde::de::Error::invalid_value(
serde::de::Unexpected::Str(&value),
&"a strictly positive number",
&"a positive number",
));
}
return Ok(Self::Percentage(float_value));
Expand All @@ -84,10 +80,10 @@ impl<'de> Deserialize<'de> for MarginValue {
&"a valid float",
)
})?;
if float_value <= 0.0 {
if float_value < 0.0 {
return Err(serde::de::Error::invalid_value(
serde::de::Unexpected::Str(&value),
&"a strictly positive float",
&"a positive number",
));
}
return Ok(Self::MinPer100Km(float_value));
Expand All @@ -102,7 +98,6 @@ impl Serialize for MarginValue {
S: serde::Serializer,
{
match self {
MarginValue::None => serializer.serialize_str("none"),
MarginValue::Percentage(value) => serializer.serialize_str(&format!("{}%", value)),
MarginValue::MinPer100Km(value) => {
serializer.serialize_str(&format!("{}min/100km", value))
Expand All @@ -122,9 +117,6 @@ mod tests {
/// Test that the `MarginValue` enum can be deserialized from a string
#[test]
fn deserialize_margin_value() {
let none: MarginValue = from_str(r#""none""#).unwrap();
assert_eq!(none, MarginValue::None);

let percentage: MarginValue = from_str(r#""10%""#).unwrap();
assert_eq!(percentage, MarginValue::Percentage(10.0));

Expand All @@ -143,9 +135,6 @@ mod tests {
/// Test that the `MarginValue` enum can be serialized to a string
#[test]
fn serialize_margin_value() {
let none = to_string(&MarginValue::None).unwrap();
assert_eq!(none, r#""none""#);

let percentage = to_string(&MarginValue::Percentage(10.0)).unwrap();
assert_eq!(percentage, r#""10%""#);

Expand All @@ -156,11 +145,11 @@ mod tests {
/// Test that Margins deserialization checks works
#[test]
fn deserialize_margins() {
let valid_margins = r#"{"boundaries":["a", "b"],"values":["none","10%","20min/100km"]}"#;
let valid_margins = r#"{"boundaries":["a", "b"],"values":["0%","10%","20min/100km"]}"#;
assert!(from_str::<Margins>(valid_margins).is_ok());
let invalid_margins = r#"{"boundaries":["a"],"values":["none","10%","20min/100km"]}"#;
let invalid_margins = r#"{"boundaries":["a"],"values":["0min/100km","10%","20min/100km"]}"#;
assert!(from_str::<Margins>(invalid_margins).is_err());
let invalid_margins = r#"{"boundaries":["a", "b"],"values":["none","10%"]}"#;
let invalid_margins = r#"{"boundaries":["a", "b"],"values":["0%","10%"]}"#;
assert!(from_str::<Margins>(invalid_margins).is_err());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
UPDATE train_schedule_v2
SET margins = REGEXP_REPLACE(margins::text, '0%', 'none', 'g')::jsonb;
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
UPDATE train_schedule_v2
SET margins = REGEXP_REPLACE(margins::text, 'none', '0%', 'g')::jsonb;
12 changes: 4 additions & 8 deletions editoast/openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3025,9 +3025,8 @@ paths:
$ref: '#/components/schemas/Comfort'
margin:
type: string
description: Can be a percentage `X%`, a time in minutes per 100 kilometer `Xmin/100km` or `None`
description: Can be a percentage `X%`, a time in minutes per 100 kilometer `Xmin/100km`
example:
- None
- 5%
- 2min/100km
nullable: true
Expand Down Expand Up @@ -7272,9 +7271,8 @@ components:
type: string
description: |-
The values of the margins. Must contains one more element than the boundaries
Can be a percentage `X%`, a time in minutes per 100 kilometer `Xmin/100km` or `none`
Can be a percentage `X%` or a time in minutes per 100 kilometer `Xmin/100km`
example:
- none
- 5%
- 2min/100km
additionalProperties: false
Expand Down Expand Up @@ -9773,9 +9771,8 @@ components:
$ref: '#/components/schemas/Comfort'
margin:
type: string
description: Can be a percentage `X%`, a time in minutes per 100 kilometer `Xmin/100km` or `None`
description: Can be a percentage `X%`, a time in minutes per 100 kilometer `Xmin/100km`
example:
- None
- 5%
- 2min/100km
nullable: true
Expand Down Expand Up @@ -11864,9 +11861,8 @@ components:
type: string
description: |-
The values of the margins. Must contains one more element than the boundaries
Can be a percentage `X%`, a time in minutes per 100 kilometer `Xmin/100km` or `none`
Can be a percentage `X%` or a time in minutes per 100 kilometer `Xmin/100km`
example:
- none
- 5%
- 2min/100km
additionalProperties: false
Expand Down
2 changes: 1 addition & 1 deletion editoast/src/tests/train_schedules/simple.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
"values": [
"5%",
"3min/100km",
"none"
"0%"
]
},
"initial_speed": 2.5,
Expand Down
2 changes: 1 addition & 1 deletion editoast/src/tests/train_schedules/simple_array.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
"values": [
"5%",
"3min/100km",
"none"
"0%"
]
},
"initial_speed": 2.5,
Expand Down
4 changes: 2 additions & 2 deletions editoast/src/views/v2/timetable/stdcm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,9 @@ pub struct STDCMRequestPayload {
/// available at least that many milliseconds after its passage.
#[serde(default)]
time_gap_after: u64,
/// Can be a percentage `X%`, a time in minutes per 100 kilometer `Xmin/100km` or `None`
/// Can be a percentage `X%`, a time in minutes per 100 kilometer `Xmin/100km`
#[serde(default)]
#[schema(value_type = Option<String>, example = json!(["None", "5%", "2min/100km"]))]
#[schema(value_type = Option<String>, example = json!(["5%", "2min/100km"]))]
margin: Option<MarginValue>,
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ const useSetupItineraryForTrainUpdate = (
arrival: arrival
? addDurationToIsoDate(trainSchedule.start_time, arrival).substring(11, 19)
: arrival,
stopFor: stopFor ? ISO8601Duration2sec(stopFor) : stopFor,
stopFor: stopFor ? ISO8601Duration2sec(stopFor).toString() : stopFor,
locked,
onStopSignal,
coordinates: stepsCoordinates[i],
Expand All @@ -128,17 +128,27 @@ const useSetupItineraryForTrainUpdate = (

const findCorrespondingMargin = (
stepId: string,
stepIndex: number,
margins: { boundaries: string[]; values: string[] }
) => {
// The first pathStep will never have its id in boundaries
if (stepIndex === 0)
return margins.values[0] === 'none' ? undefined : margins.values[0];

const marginIndex = margins.boundaries.findIndex(
(boundaryId) => boundaryId === stepId
);

return marginIndex !== -1 ? margins.values[marginIndex + 1] : undefined;
};

if (trainSchedule.margins) {
updatedPathSteps.forEach((step) => {
step.theoreticalMargin = findCorrespondingMargin(step.id, trainSchedule.margins!);
updatedPathSteps.forEach((step, index) => {
step.theoreticalMargin = findCorrespondingMargin(
step.id,
index,
trainSchedule.margins!
);
});
}

Expand Down
2 changes: 1 addition & 1 deletion front/src/applications/stdcm/utils/createMargin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { StandardAllowance } from 'reducers/osrdconf/types';

export default function createMargin(margin: StandardAllowance | undefined): string {
if (!margin || !margin.value) {
return 'None';
return '0%';
}
switch (margin.type) {
case 'time_per_distance':
Expand Down
4 changes: 2 additions & 2 deletions front/src/common/api/generatedEditoastApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1748,7 +1748,7 @@ export type PostV2TimetableByIdStdcmApiArg = {
id: number;
body: {
comfort: Comfort;
/** Can be a percentage `X%`, a time in minutes per 100 kilometer `Xmin/100km` or `None` */
/** Can be a percentage `X%`, a time in minutes per 100 kilometer `Xmin/100km` */
margin?: string | null;
/** By how long we can shift the departure time in milliseconds
Deprecated, first step data should be used instead */
Expand Down Expand Up @@ -3673,7 +3673,7 @@ export type TrainScheduleBase = {
margins?: {
boundaries: string[];
/** The values of the margins. Must contains one more element than the boundaries
Can be a percentage `X%`, a time in minutes per 100 kilometer `Xmin/100km` or `none` */
Can be a percentage `X%` or a time in minutes per 100 kilometer `Xmin/100km` */
values: string[];
};
options?: {
Expand Down
2 changes: 2 additions & 0 deletions front/src/modules/pathfinding/hook/usePathfinding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -249,12 +249,14 @@ export const usePathfindingV2 = (
'uic' in step && suggestedOp.uic === step.uic && suggestedOp.ch === step.ch
);

const theoreticalMargin = i === 0 ? '0%' : step.theoreticalMargin;
const stopFor = i === pathSteps.length - 1 && !step.stopFor ? '0' : step.stopFor;

return {
...step,
positionOnPath: pathfindingResult.path_item_positions[i],
stopFor,
theoreticalMargin,
...(correspondingOp && {
name: correspondingOp.name,
uic: correspondingOp.uic,
Expand Down
7 changes: 6 additions & 1 deletion front/src/modules/timesStops/TimesStops.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,12 @@ const TimesStops = ({ pathProperties, pathSteps = [], startTime }: TimesStopsPro
setTimesStopsSteps(row);
} else {
rowData.isMarginValid = true;
if (op.fromRowIndex === 0) rowData.arrival = null;
if (op.fromRowIndex === 0) {
rowData.arrival = null;
// As we put 0% by default for origin's margin, if the user removes a margin without
// replacing it to 0% (undefined), we change it to 0%
if (!rowData.theoreticalMargin) rowData.theoreticalMargin = '0%';
}
dispatch(upsertViaFromSuggestedOP(rowData as SuggestedOP));
}
}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,17 @@ const formatMargin = (pathSteps: PathStep[]): Margin | undefined => {
const boundaries: string[] = [];
const values: string[] = [];

pathSteps.forEach((step, index) => {
pathSteps.forEach(({ id, theoreticalMargin }, index) => {
if (index === 0) {
values.push(step.theoreticalMargin || 'none');
values.push(theoreticalMargin || '0%');
} else if (index === pathSteps.length - 1) {
if (values.length === 1 && values[0] !== 'none') {
boundaries.push(step.id);
values.push('none');
if (values.length === 1 && values[0] !== '0%' && values[0] !== '0min/100km') {
boundaries.push(id);
values.push('0%');
}
} else if (step.theoreticalMargin) {
boundaries.push(step.id);
values.push(step.theoreticalMargin);
} else if (theoreticalMargin) {
boundaries.push(id);
values.push(theoreticalMargin);
}
});

Expand Down
2 changes: 1 addition & 1 deletion tests/fuzzer/fuzzer_v2.py
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ def _make_stdcm_payload(path: List[Tuple[str, float]], rolling_stock: int) -> Di
"time_gap_after": random.randint(0, 600_000),
"steps": [_convert_stop_stdcm(stop) for stop in path],
"comfort": "STANDARD",
"margin": "None",
"margin": "0%",
}
res["steps"][-1]["duration"] = 1 # Force a stop at the end
allowance_value = _make_random_margin_value()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
}
],
"comfort": "STANDARD",
"margin": "None"
"margin": "0%"
},
"prelude": [
{
Expand Down Expand Up @@ -132,7 +132,7 @@
"2"
],
"values": [
"None",
"0%",
"7%"
]
},
Expand Down Expand Up @@ -171,12 +171,6 @@
"initial_speed": 0,
"labels": [],
"constraint_distribution": "MARECO",
"margins": {
"boundaries": [],
"values": [
"None"
]
},
"options": {
"use_electrical_profiles": false
},
Expand Down Expand Up @@ -207,12 +201,6 @@
"initial_speed": 0,
"labels": [],
"constraint_distribution": "STANDARD",
"margins": {
"boundaries": [],
"values": [
"None"
]
},
"options": {
"use_electrical_profiles": false
},
Expand Down Expand Up @@ -243,12 +231,6 @@
"initial_speed": 0,
"labels": [],
"constraint_distribution": "STANDARD",
"margins": {
"boundaries": [],
"values": [
"None"
]
},
"options": {
"use_electrical_profiles": false
},
Expand Down Expand Up @@ -299,7 +281,7 @@
"2"
],
"values": [
"None",
"0%",
"8%"
]
},
Expand Down
Loading
Loading