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

stdcm: only simulate edges up to the next stop #3667

Merged
merged 1 commit into from
Apr 4, 2023
Merged

Conversation

eckter
Copy link
Contributor

@eckter eckter commented Mar 24, 2023

When computing stdcm edges, we now only go up to the first stop. Before this, we always simulated an envelope over the whole route and sometimes detected invalid conflicts after the end of the path. Fixes a bug reproduced in a couple unit tests that were disabled.

First step for #3301.

To do this, there has been a problem: we sometimes need to simulate a 0-length path. To do this, I have removed the assertion in EnvelopePart about single point parts. The alternative would have been a new class implementing an interface shared with Envelope, but it would be a heavier change. I'd like feedback on this.

@eckter eckter requested a review from a team as a code owner March 24, 2023 13:34
@codecov
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Merging #3667 (d44d7b1) into dev (76f95fe) will increase coverage by 0.03%.
The diff coverage is 79.16%.

@@             Coverage Diff              @@
##                dev    #3667      +/-   ##
============================================
+ Coverage     69.80%   69.83%   +0.03%     
- Complexity     2002     2011       +9     
============================================
  Files           438      438              
  Lines         21899    21912      +13     
  Branches       1658     1661       +3     
============================================
+ Hits          15286    15302      +16     
+ Misses         5818     5817       -1     
+ Partials        795      793       -2     
Flag Coverage Δ
core 79.13% <79.16%> (+0.06%) ⬆️

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

Impacted Files Coverage Δ
.../java/fr/sncf/osrd/envelope/part/EnvelopePart.java 80.50% <0.00%> (ø)
...d/infra_state/implementation/TrainPathBuilder.java 81.94% <0.00%> (-1.39%) ⬇️
...in/java/fr/sncf/osrd/stdcm/graph/DelayManager.java 87.80% <ø> (-2.44%) ⬇️
.../osrd/envelope_sim/pipelines/MaxSpeedEnvelope.java 90.24% <40.00%> (+0.50%) ⬆️
...ava/fr/sncf/osrd/stdcm/graph/STDCMEdgeBuilder.java 93.90% <100.00%> (ø)
...ava/fr/sncf/osrd/stdcm/graph/STDCMSimulations.java 97.95% <100.00%> (+0.45%) ⬆️
.../sncf/osrd/stdcm/graph/STDCMStandardAllowance.java 76.31% <100.00%> (ø)
...main/java/fr/sncf/osrd/stdcm/graph/STDCMUtils.java 85.45% <100.00%> (+0.54%) ⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@eckter eckter force-pushed the ech/stdcm-partial-edges branch 2 times, most recently from 9c8e140 to 60dac03 Compare March 28, 2023 08:16
Copy link
Contributor

@Khoyo Khoyo left a comment

Choose a reason for hiding this comment

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

LGTM on code quality. I trust you on STDCM semantics ;)

@eckter eckter merged commit 9492252 into dev Apr 4, 2023
@eckter eckter deleted the ech/stdcm-partial-edges branch April 4, 2023 13:27
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.

3 participants