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.

Sign up for GitHub

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

Jump to bottom

core: support arrival on stop signal for conflict detection #7322

Merged
merged 4 commits into from
May 31, 2024
Merged

core: support arrival on stop signal for conflict detection #7322

merged 4 commits into from
May 31, 2024

Conversation

bougue-pe
Copy link
Contributor

@bougue-pe bougue-pe commented Apr 25, 2024 •
edited
Loading

TODO:

  • rebase on TSv2's work (conflicts)
  • consider first comments (in a separate commit) ?
  • branch on TSv1 (on_stop_signal = true if stop_for is not None) + manual test ? > Not targetted
  • port work for TSv2 (copy code, use actual flag from editoast) + manual test ?
  • update tests to cover cases imagined in Create a test generating spacing conflicts #7044 and Create test generating path (routing) conflicts #7045, check also if these tests are triggering TSv2's code. -> will be done in a subsequent PR
  • decide whether adding e2e test is a good idea (probably only integration tests).

@codecov-commenter
Copy link

codecov-commenter commented Apr 25, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 84.51178% with 46 lines in your changes are missing coverage. Please review.

Project coverage is 29.06%. Comparing base (23ed3c3) to head (9179115).

Files Patch % Lines
...conflicts/IncrementalRequirementEnvelopeAdapter.kt 25.00% 4 Missing and 5 partials ⚠️
.../sncf/osrd/signal_projection/SignalProjectionV2.kt 0.00% 9 Missing ⚠️
...va/fr/sncf/osrd/standalone_sim/SignalProjection.kt 0.00% 7 Missing ⚠️
...ain/java/fr/sncf/osrd/conflicts/IncrementalPath.kt 76.19% 1 Missing and 4 partials ⚠️
...fr/sncf/osrd/conflicts/SpacingResourceGenerator.kt 90.90% 2 Missing and 3 partials ⚠️
...otlin/fr/sncf/osrd/stdcm/graph/STDCMPathfinding.kt 50.00% 1 Missing and 2 partials ⚠️
...api_v2/project_signals/SignalProjectionResponse.kt 0.00% 2 Missing ⚠️
editoast/src/views/timetable/import.rs 0.00% 2 Missing ⚠️
tests/conftest.py 77.77% 2 Missing ⚠️
...n/fr/sncf/osrd/api/api_v2/stdcm/STDCMEndpointV2.kt 0.00% 1 Missing ⚠️
... and 1 more

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

Additional details and impacted files 
@@             Coverage Diff              @@
##                dev    #7322      +/-   ##
============================================
+ Coverage     28.99%   29.06%   +0.06%     
- Complexity     2021     2028       +7     
============================================
  Files          1223     1225       +2     
  Lines        149045   149222     +177     
  Branches       2934     2962      +28     
============================================
+ Hits          43218    43365     +147     
- Misses       104087   104107      +20     
- Partials       1740     1750      +10
Flag Coverage Δ
core 75.12% <83.85%> (+0.13%) ⬆️
editoast 71.92% <0.00%> (-0.04%) ⬇️
front 9.98% <100.00%> (+<0.01%) ⬆️
gateway 2.41% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 72.54% <94.87%> (+0.69%) ⬆️

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 changed the title core: support arrival on stop signal in conflicts core: support arrival on stop signal for conflict detection Apr 30, 2024
woshilapin
woshilapin reviewed May 6, 2024
View reviewed changes
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.

Just a first pass of comments for the moment, trying to make sense of all of this.

core/src/main/java/fr/sncf/osrd/conflicts/IncrementalPath.kt Show resolved Hide resolved
core/src/main/java/fr/sncf/osrd/standalone_sim/ScheduleMetadataExtractor.kt Outdated Show resolved Hide resolved
core/src/main/java/fr/sncf/osrd/standalone_sim/ScheduleMetadataExtractor.kt Show resolved Hide resolved
core/src/main/java/fr/sncf/osrd/conflicts/IncrementalRequirementEnvelopeAdapter.kt Outdated Show resolved Hide resolved
core/src/main/kotlin/fr/sncf/osrd/stdcm/infra_exploration/InfraExplorer.kt Outdated Show resolved Hide resolved
core/src/main/java/fr/sncf/osrd/conflicts/SpacingResourceGenerator.kt Outdated Show resolved Hide resolved
core/src/main/java/fr/sncf/osrd/conflicts/SpacingResourceGenerator.kt Outdated Show resolved Hide resolved
@woshilapin woshilapin force-pushed the core/arrival_on_stop_signal branch from 3766a56 to ce71668 Compare May 6, 2024 13:59
@woshilapin woshilapin force-pushed the core/arrival_on_stop_signal branch 3 times, most recently from 41267d6 to 176f4ff Compare May 22, 2024 15:28
@bougue-pe bougue-pe force-pushed the core/arrival_on_stop_signal branch 11 times, most recently from ffe32d1 to 59f7f67 Compare May 29, 2024 23:33
@woshilapin woshilapin force-pushed the core/arrival_on_stop_signal branch 2 times, most recently from 5ef4b4f to 9737fdb Compare May 30, 2024 08:16
@bougue-pe bougue-pe force-pushed the core/arrival_on_stop_signal branch 2 times, most recently from a97f97f to 5c71726 Compare May 30, 2024 08:47
@woshilapin woshilapin marked this pull request as ready for review May 30, 2024 13:30
@woshilapin woshilapin requested review from a team as code owners May 30, 2024 13:30
@woshilapin woshilapin requested review from multun and bloussou May 30, 2024 13:30
@bougue-pe bougue-pe force-pushed the core/arrival_on_stop_signal branch from 88e815c to c4f4f8d Compare May 30, 2024 14:03
@bougue-pe bougue-pe force-pushed the core/arrival_on_stop_signal branch from 3ad325e to 2a666fc Compare May 30, 2024 21:00
@woshilapin woshilapin force-pushed the core/arrival_on_stop_signal branch from 2a666fc to 404eb89 Compare May 31, 2024 07:48
@bougue-pe bougue-pe force-pushed the core/arrival_on_stop_signal branch 4 times, most recently from 9de94ec to c3bdc7a Compare May 31, 2024 12:56
flomonster
flomonster approved these changes May 31, 2024
View reviewed changes
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

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

Reviewed Editoast only

eckter
eckter reviewed May 31, 2024
View reviewed changes
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.

LGTM for tests (which is where my review was actually required), but there's some weird stuff going on in core. Mostly minor comments but there's one that requires careful double-checking

core/src/main/java/fr/sncf/osrd/conflicts/IncrementalRequirementEnvelopeAdapter.kt Outdated Show resolved Hide resolved
core/src/main/java/fr/sncf/osrd/conflicts/SpacingResourceGenerator.kt Show resolved Hide resolved
core/src/main/kotlin/fr/sncf/osrd/stdcm/infra_exploration/InfraExplorer.kt Outdated Show resolved Hide resolved
core/src/main/kotlin/fr/sncf/osrd/stdcm/infra_exploration/InfraExplorer.kt Outdated Show resolved Hide resolved
core/src/main/kotlin/fr/sncf/osrd/stdcm/infra_exploration/InfraExplorer.kt Outdated Show resolved Hide resolved
@bougue-pe bougue-pe requested review from eckter, woshilapin and Khoyo May 31, 2024 17:32
woshilapin and others added 4 commits May 31, 2024 21:13
@woshilapin @bougue-pe
core: test arrival on stop signal support
91c7f1f 
So far, there is a test that is supposed to provide a situation where:
- train A goes from SA5 to SD2 through SC5, with a stop in SC5
- train B goes from SA5 to SD2 through SC4, with no stop but leaving after train A
- the full path is clamped at SC1 for train A to avoid a conflict on PC0
- the full path is clamped at SC0 for train B to avoid a conflict on PC0

See https://osrd.fr/en/docs/explanation/models/data-models-full-example/svg_diagrams/small_infra_op_points.drawio.en.svg
for a reference of the infrastructure.
@woshilapin @bougue-pe
core: renaming of counts (not using index)
1adb6f7
@woshilapin @Khoyo
@multun @bougue-pe
core, editoast: support arrival-on-stop-signal for conflict detection
c38362e 
Done for spacing and routing requirements
Main target is TrainScheduleV2 (STDCM and TSv1 only minimal)
Update tests accordingly

Also:
* add PathOffsetBuilder between Path and TravelledPath
* fix offset types of ZoneUpdate and SignalSighting
* get rid of test cases with empty stops in InfraExplorer

Co-authored-by: Younes Khoudli <younes.khoudli@epita.fr>
Co-authored-by: Victor "multun" Collod <victor.collod@epita.fr>
Co-authored-by: Pierre-Etienne Bougué <bougue.pe@proton.me>
@bougue-pe
tests: add integration tests for arrival on stop signal
9179115
@bougue-pe bougue-pe force-pushed the core/arrival_on_stop_signal branch from 7412601 to 9179115 Compare May 31, 2024 19:14
@bougue-pe bougue-pe added this pull request to the merge queue May 31, 2024
Merged via the queue into dev with commit 3203aa3 May 31, 2024
17 checks passed
@bougue-pe bougue-pe deleted the core/arrival_on_stop_signal branch May 31, 2024 20:21
This was referenced Jun 4, 2024
Merged
Closed
Closed
Closed
@woshilapin woshilapin mentioned this pull request Jun 20, 2024
Closed
1 task
@bougue-pe bougue-pe mentioned this pull request Sep 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flomonster flomonster flomonster approved these changes

@multun multun multun approved these changes

@eckter eckter eckter approved these changes

@Khoyo Khoyo Khoyo approved these changes

@bloussou bloussou Awaiting requested review from bloussou bloussou is a code owner automatically assigned from OpenRailAssociation/osrd-tests

@woshilapin woshilapin Awaiting requested review from woshilapin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@bougue-pe @codecov-commenter @Khoyo @woshilapin @eckter @multun @flomonster