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: change pathfinding errors on rolling-stock constraints #7953

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

Conversation

bougue-pe
Copy link
Contributor

@bougue-pe bougue-pe commented Jul 4, 2024

Change interface for pathfinding errors on rolling-stock constraints and project RS constraints on the relaxed path.
⚠️ this breaks core interface and will be handled in another PR by editoast.

TODO:

  • test with holes between blocked ranges
  • test incompatible gauge
  • check if validate PR response is/should be done

Fix: #7719

@bougue-pe bougue-pe requested review from sim51 and Erashin July 4, 2024 16:01
@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2024

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

Codecov Report

Attention: Patch coverage is 81.30081% with 23 lines in your changes missing coverage. Please review.

Project coverage is 28.09%. Comparing base (e65899e) to head (fe3b779).

Files Patch % Lines
.../api_v2/pathfinding/PathfindingBlocksEndpointV2.kt 86.02% 4 Missing and 9 partials ⚠️
...api/api_v2/pathfinding/PathfindingBlockResponse.kt 57.14% 2 Missing and 7 partials ⚠️
...r/sncf/osrd/signaling/impl/MockSigSystemManager.kt 0.00% 1 Missing ⚠️

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

Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #7953      +/-   ##
============================================
+ Coverage     28.06%   28.09%   +0.03%     
- Complexity     2075     2077       +2     
============================================
  Files          1289     1289              
  Lines        157764   157834      +70     
  Branches       3121     3134      +13     
============================================
+ Hits          44281    44350      +69     
+ Misses       111606   111592      -14     
- Partials       1877     1892      +15     
Flag Coverage Δ
core 75.18% <81.30%> (+0.15%) ⬆️
editoast 70.78% <ø> (-0.02%) ⬇️
front 9.94% <ø> (ø)
gateway 2.34% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 72.93% <ø> (ø)

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.

@bougue-pe bougue-pe force-pushed the peb/core/incompatible_rolling_stock_constraints_pathfinding branch 2 times, most recently from 0e4eb4c to 0a2d394 Compare July 5, 2024 16:11
@bougue-pe bougue-pe force-pushed the peb/core/incompatible_rolling_stock_constraints_pathfinding branch 5 times, most recently from 2f0ca04 to 2f67537 Compare July 9, 2024 12:58
@bougue-pe bougue-pe force-pushed the peb/core/incompatible_rolling_stock_constraints_pathfinding branch from abccc44 to 8ee6553 Compare July 11, 2024 09:07
@bougue-pe bougue-pe requested a review from Erashin July 11, 2024 09:29
@bougue-pe bougue-pe changed the title core: change interface for pathfinding errors on rolling-stock constraints core: change pathfinding errors on rolling-stock constraints Jul 11, 2024
@bougue-pe bougue-pe force-pushed the peb/core/incompatible_rolling_stock_constraints_pathfinding branch from 8ee6553 to d20609e Compare July 11, 2024 12:22
waypoints: ArrayList<Collection<PathfindingEdgeLocationId<Block>>>,
constraints: List<PathfindingConstraint<Block>>,
remainingDistanceEstimators: List<AStarHeuristicId<Block>>,
timeout: Double?
Copy link
Contributor

@Erashin Erashin Jul 12, 2024

Choose a reason for hiding this comment

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

Not sure we need the timeout. Timeout is used to prevent the algorithm to compute for too much time when it doesn't find a solution. Since we've arrived here, it means the constrained pathfinding has already taken less than 2min. As a result, there's no reason the unconstrained pathfinding should take more than 2min.

Edit: might be fine to actually keep the timeout here, but having it be only 2min - basePathfindingTime is harsh.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we've arrived here, it means the constrained pathfinding has already taken less than 2min. As a result, there's no reason the unconstrained pathfinding should take more than 2min.

It can, the unconstrained pathfinding has more edges to explore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After first discussions: we want to be able to distinguish a timeout using initial (real) constraints and a timeout unconstrained if we have an "unconstrained" timeout.
Now the question is (@bloussou ?): What timeout do we want on unconstrained PF (launched to "explain" failure) ?

  1. shared with the constrained PF
  2. equal to the constrained PF
  3. no timeout

Copy link
Contributor

@bloussou bloussou Jul 15, 2024

Choose a reason for hiding this comment

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

Is it triggered for each failing PF ?

Copy link
Contributor

@Erashin Erashin Jul 15, 2024

Choose a reason for hiding this comment

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

Yes. For each failing PF (except if it takes >2min and fails due to timeout), the unconstrained PF is triggered, to find out why it failed (ex: due to electrification constraints), the explanation is returned and will be displayed in the front end of the app (back end part of this feature is this PR).
If your question was if the timeout was triggered for every failing PF, the answer is no, it depends.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may not care about explaining PF failure (RS constraints) when using the timeout (BASIC import).
It would let more freedom and maybe aim at a quicker fail, so a mutualized timeout?

Copy link
Contributor

@bloussou bloussou Jul 16, 2024

Choose a reason for hiding this comment

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

okay lets go for a mutualized timeout I think

@bougue-pe bougue-pe force-pushed the peb/core/incompatible_rolling_stock_constraints_pathfinding branch from d20609e to 65f3fe1 Compare July 15, 2024 05:36
@bougue-pe bougue-pe force-pushed the peb/core/incompatible_rolling_stock_constraints_pathfinding branch from 65f3fe1 to fe3b779 Compare July 15, 2024 06:12
@bougue-pe bougue-pe marked this pull request as ready for review July 15, 2024 06:16
@bougue-pe bougue-pe requested a review from a team as a code owner July 15, 2024 06:16
@bougue-pe bougue-pe requested a review from Khoyo July 15, 2024 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return list of incompatible RS constraints in path finding (back)
7 participants