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: stdcm: conflict detection integration #6171

Merged
merged 39 commits into from
Mar 14, 2024
Merged

Conversation

eckter
Copy link
Contributor

@eckter eckter commented Dec 20, 2023

Fix #4484

The reason for such a massive PR is that we needed to have a stable testable product to be confident in the implementation. There has been several points where we thought "this part can be merged into dev" and yet we still had to make deep changes later on.

Change summary:

  1. Change the way we navigate the infra in STDCM
    1. Instead of going from block to block manually, we have an opaque object (InfraExplorer) that represent the different possible paths.
    2. Internally, it keeps track of routes and blocks from the start of the path, and can extend beyond the current point being evaluated by the pathfinding.
    3. Another object built upon the first one can also keep track of past simulations / envelopes in a given path
  2. Change the way conflicts are detected in STDCM
    1. Instead of manually identifying the available blocks, we call SpacingRequirementAutomaton and compare the results using conflicts.kt tools
    2. To do this, we needed a few tweaks to conflicts.kt to get the data we need. Because resource uses are supposed to be opaque, we needed to add a new method any time we needed to get something like minimum delay to add.
  3. Tests have been adapted
    1. The old classes used to identify available blocks have been kept and moved to the test modules. They are used to test STDCM while abstracting away the actual signaling. It's extremely useful to build the test scenarios in straightforward way.
    2. The conflict detection integration has mostly been tested at the BlockAvailabilityInterface level: we don't check how the conflicts are avoided (that's done in the tests mentioned above), but we check that the conflicting times are properly identified.
  4. Optimizations (not exhaustive because there are smaller ones):
    1. Cache lots of objects and results
    2. Avoid unnecessary object instantiations and computations
    3. Spacing resource requirements: use a binary search to find the last required zone
    4. Avoid instantiating edges that would be considered "already visited"

About the git history: I did my best to clean it up, but there's only so much that can be done. Commit-by-commit review is not recommended for STDCM files. There has been whole classes that have been split and re-merged and tweaked, and too much stuff happened in the meantime for proper fixups/squashs. Changes that are outside of STDCM scope have clear atomic commits (generally bugfixes), but new files and methods should be reviewed in one go. I've kept most of the history because it may still help a little with the review, but I'll do a more agressive squash towards the end of the process. The plan is to squash everything up to the point where we transition to bugfixes / optimizations only. Once reviews start coming, we'll stick to fixup commits and avoid rebases.

This has been tested with extensive fuzzing on both small_infra and France, including a new assertion that the new train isn't conflicting with the timetable. The bugs that aren't already present on dev have been fixed. There's also a fairly large amount of unit tests in core (IIRC more than half of the line diff is located in test files). I also tested manually on small_infra and on France, but not in depth. We need to test once again because of the optimization changes, but we can still start the reviews

Additional notes:

  1. Some implementations have poor performances. We did our best to keep the PR straightforward, optimization will likely be planned for the next PI
    1. edit: we do have performances issues, but the poor complexity in the new classes have nothing to do with it
  2. Similarly, for the most part we only consider the spacing conflicts. Routing conflicts will be added later on (it should be mostly painless)
  3. Deadline: end of this week :'( (16/03)
  4. Once approved, I'll update the documentation on the website

Copy link

codecov bot commented Dec 20, 2023

Codecov Report

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

Project coverage is 28.64%. Comparing base (fbc0817) to head (55d0d6a).

Files Patch % Lines
.../src/main/java/fr/sncf/osrd/conflicts/Conflicts.kt 56.62% 32 Missing and 4 partials ⚠️
...fr/sncf/osrd/sim_infra/utils/PathPropertiesView.kt 48.48% 16 Missing and 1 partial ⚠️
.../kotlin/fr/sncf/osrd/sim_infra/utils/InfraUtils.kt 75.40% 9 Missing and 6 partials ⚠️
.../main/kotlin/fr/sncf/osrd/stdcm/graph/STDCMNode.kt 25.00% 6 Missing ⚠️
...sncf/osrd/stdcm/infra_exploration/InfraExplorer.kt 94.69% 1 Missing and 5 partials ⚠️
...ain/java/fr/sncf/osrd/conflicts/IncrementalPath.kt 90.00% 1 Missing and 3 partials ⚠️
...conflicts/IncrementalRequirementEnvelopeAdapter.kt 55.55% 0 Missing and 4 partials ⚠️
...fr/sncf/osrd/conflicts/SpacingResourceGenerator.kt 93.84% 1 Missing and 3 partials ⚠️
.../main/kotlin/fr/sncf/osrd/stdcm/graph/STDCMEdge.kt 82.60% 1 Missing and 3 partials ⚠️
.../kotlin/fr/sncf/osrd/utils/AppendOnlyLinkedList.kt 91.42% 1 Missing and 2 partials ⚠️
... and 10 more
Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #6171      +/-   ##
============================================
+ Coverage     28.46%   28.64%   +0.17%     
- Complexity     2179     2223      +44     
============================================
  Files          1055     1059       +4     
  Lines        131222   131610     +388     
  Branches       2602     2646      +44     
============================================
+ Hits          37356    37702     +346     
- Misses        92353    92387      +34     
- Partials       1513     1521       +8     
Flag Coverage Δ
core 79.81% <87.31%> (+0.37%) ⬆️
editoast 74.89% <ø> (-0.03%) ⬇️
front 8.92% <ø> (ø)
gateway 2.49% <ø> (ø)
railjson_generator 87.26% <ø> (ø)
tests 83.07% <ø> (ø)

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.

@eckter eckter force-pushed the stdcm-signaling-integration branch 7 times, most recently from 879f6b0 to 5498ea9 Compare December 22, 2023 09:58
@eckter eckter force-pushed the stdcm-signaling-integration branch 8 times, most recently from d63f81b to 887b6f8 Compare January 4, 2024 14:48
Copy link
Contributor

@Erashin Erashin left a comment

Choose a reason for hiding this comment

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

Lgtm up to this point :)

@Erashin Erashin linked an issue Jan 4, 2024 that may be closed by this pull request
@eckter eckter force-pushed the stdcm-signaling-integration branch 4 times, most recently from 9686abb to 2bd951e Compare January 4, 2024 15:52
@eckter eckter changed the title core: stdcm: use new interface for infra exploration core: stdcm: conflict detection integration Jan 4, 2024
@eckter eckter force-pushed the stdcm-signaling-integration branch 3 times, most recently from 8da82fe to 8697fdc Compare January 16, 2024 10:40
@Erashin Erashin force-pushed the stdcm-signaling-integration branch 3 times, most recently from f1475e9 to 121a444 Compare February 5, 2024 10:34
eckter added 22 commits March 14, 2024 10:16
We used to not consider the blocks on the routes before the
start one, but as a result the conversions to/from
travelled path offsets were wrong
The map returned from the signaling simulator is
sparse, which IdxMap can *not* handle in an efficient way
The value is arbitrary but seems to fit for BAL,
I tried to increase it one by one until no exception
is thrown.
core: spacing-generator: fix accidental n^2 complexity on repeated calls

core: signaling simulator: avoid redundant block list copy

The previous version inputed the whole path to each call
of the signaling simulation, resulting on a very poor
scaling on long paths in stdcm
Repeated edges at different locations of the same
block were considered identical
We could sometimes enter infinite loops on stop times
We used to copy every data about the past elements of the
path when we explore diverging paths, we now keep
linked list that don't duplicate data
When both are equal we enter into the "epsilon error"
ranges of float operations in `callbacks.arrivalTimeInRange`
@eckter eckter force-pushed the stdcm-signaling-integration branch from 79b210c to 55d0d6a Compare March 14, 2024 09:16
@eckter eckter enabled auto-merge March 14, 2024 09:25
@eckter eckter added this pull request to the merge queue Mar 14, 2024
Merged via the queue into dev with commit 75bf2e9 Mar 14, 2024
22 checks passed
@eckter eckter deleted the stdcm-signaling-integration branch March 14, 2024 09:44
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.

meta: core: stdcm: use the new conflict detection
3 participants