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

core: editoast: front: add flag to ignore infra speed limits #10681

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

axrolld
Copy link
Contributor

@axrolld axrolld commented Feb 5, 2025

Add a flag to ignore infra speed limits in a standalone simulation.

This will be useful in BASIC import to fix late trains for the moment.

Closes #10587

@github-actions github-actions bot added area:railjson Work on Proposed Unified Rail Assets Data Exchange Format area:core Work on Core Service area:editoast Work on Editoast Service labels Feb 5, 2025
@codecov-commenter
Copy link

codecov-commenter commented Feb 5, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.93%. Comparing base (fd615e7) to head (a237b67).
Report is 76 commits behind head on dev.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #10681      +/-   ##
==========================================
+ Coverage   81.81%   81.93%   +0.11%     
==========================================
  Files        1073     1079       +6     
  Lines      106938   107376     +438     
  Branches      722      737      +15     
==========================================
+ Hits        87490    87976     +486     
+ Misses      19409    19360      -49     
- Partials       39       40       +1     
Flag Coverage Δ
editoast 74.27% <ø> (+0.09%) ⬆️
front 89.47% <ø> (+0.11%) ⬆️
gateway 2.18% <ø> (ø)
osrdyne 3.28% <ø> (ø)
railjson_generator 87.50% <ø> (ø)
tests 88.14% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot added the area:front Work on Standard OSRD Interface modules label Feb 5, 2025
@woshilapin woshilapin force-pushed the ard/flag-speed-limits branch from e834a62 to 37324e3 Compare February 5, 2025 11:03
@axrolld axrolld force-pushed the ard/flag-speed-limits branch 3 times, most recently from 31ffe44 to 210ba0e Compare February 5, 2025 14:38
@axrolld axrolld force-pushed the ard/flag-speed-limits branch from 210ba0e to dd7835a Compare February 5, 2025 16:37
Signed-off-by: Alex Rolland <[email protected]>
@axrolld axrolld force-pushed the ard/flag-speed-limits branch from e70e492 to b29487e Compare February 6, 2025 10:56
@axrolld axrolld marked this pull request as ready for review February 6, 2025 15:40
@axrolld axrolld requested review from a team as code owners February 6, 2025 15:40
@axrolld axrolld requested a review from eckter February 6, 2025 15:40
@axrolld axrolld changed the title core: editoast: add flag to ignore infra speed limits core: editoast: front: add flag to ignore infra speed limits Feb 6, 2025
Copy link
Contributor

@eckter eckter left a comment

Choose a reason for hiding this comment

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

Nice job!

@@ -143,5 +143,6 @@ class SimulationPowerRestrictionItem(
)

class TrainScheduleOptions(
@Json(name = "use_electrical_profiles") val useElectricalProfiles: Boolean
@Json(name = "use_electrical_profiles") val useElectricalProfiles: Boolean,
@Json(name = "ignore_infra_speed_limits") val ingoreInfraSpeedLimits: Boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Suggested change
@Json(name = "ignore_infra_speed_limits") val ingoreInfraSpeedLimits: Boolean
@Json(name = "ignore_infra_speed_limits") val ignoreInfraSpeedLimits: Boolean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch thanks !

@@ -143,5 +143,6 @@ class SimulationPowerRestrictionItem(
)

class TrainScheduleOptions(
@Json(name = "use_electrical_profiles") val useElectricalProfiles: Boolean
@Json(name = "use_electrical_profiles") val useElectricalProfiles: Boolean,
@Json(name = "ignore_infra_speed_limits") val ingoreInfraSpeedLimits: Boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to invert the logic to be consistent with use_electrical_profiles. We could make it optional as well, and use them if unspecified.

@@ -59,6 +59,8 @@ fun computeMRSP(
* @param rsLength length of the rolling stock (m)
* @param addRollingStockLength whether the rolling stock length should be taken into account in the
* computation.
* @param ignoreInfraSpeedLimits whether the speed limits coming from the infrastructure should be ignored in the
* computaion
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* computaion
* computation

attrs,
doubleArrayOf(start, end),
doubleArrayOf(speed, speed)
if (!ignoreInfraSpeedLimits) { //if we take into account speedlimits coming from the infrastructure
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd have preferred a way to avoid this large if block.

Maybe something like this, what do you think?

    val speedLimitProperties =
        if (ignoreInfraSpeedLimits) distanceRangeMapOf()
        else path.getSpeedLimitProperties(trainTag, temporarySpeedLimitManager)
    for (speedLimitPropertyRange in speedLimitProperties) {
        // ...
    }

@@ -72,36 +74,39 @@ fun computeMRSP(
trainTag: String?,
temporarySpeedLimitManager: TemporarySpeedLimitManager?,
safetySpeedRanges: DistanceRangeMap<Speed>? = null,
ignoreInfraSpeedLimits: Boolean = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

The parameter isn't set.

It should be done in StandaloneSimulation.kt:71. And it should also be added to the other version of the method which takes a rolling stock as parameter (this file, line 35).

Signed-off-by: Alex Rolland <[email protected]>
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

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

Looks ok for editoast but something weird showed up. Since it's not part of your PR, feel free to discard it, but if it could be fixed with it, that'd be great.

"""Optional arguments :
- `ignore_electrical_profiles` : ignore the electrical profiles for the standalone simulation
- `ignore_infra_speed_limits` : ignore the speed limits coming from the infrastructure for the standalone simulation
"""

ignore_electrical_profiles: bool = Field(
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, something is fishy. In editoast, this schema is defined as use_electrical_profiles...

It was added in osrd_schemas in 65936d6#diff-d5864b67bb112b9568c11e82046affbf1336af068b01ea186f7f7e39bf39b20eR139-R142 as ignore_electrical_profiles and was added in editoast in 79c5caa#diff-64eae74a5b61bec4f050765098f382e536496ec9b5917caa1385722f99ffdf9fR157 as use_electrical_profiles.

cc @flomonster @Castavo any idea what might have happened and what is the best way to fix that (sorry, both of your names showed up in git blame)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core Work on Core Service area:editoast Work on Editoast Service area:front Work on Standard OSRD Interface modules area:railjson Work on Proposed Unified Rail Assets Data Exchange Format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a flag to enable ignoring speed limits
4 participants