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: update start of free block in signal projection (space-time chart) when stopping on closed signal #9661

Merged
merged 5 commits into from
Nov 27, 2024

Conversation

bougue-pe
Copy link
Contributor

@bougue-pe bougue-pe commented Nov 8, 2024

Fix #8814

This is done by amending signal-sightings with stop-ending (minus margin) when stop is on closed signal

Also:

  • Adds corresponding integration test

A complete comment up-to-date is also worth reading: #9661 (comment)

🔍 review by commit is probably easier

@bougue-pe bougue-pe requested a review from Khoyo November 8, 2024 15:42
@bougue-pe bougue-pe requested review from a team as code owners November 8, 2024 15:42
@bougue-pe bougue-pe requested a review from bloussou November 8, 2024 15:42
@github-actions github-actions bot added the area:editoast Work on Editoast Service label Nov 8, 2024
@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2024

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

Codecov Report

Attention: Patch coverage is 88.88889% with 3 lines in your changes missing coverage. Please review.

Project coverage is 38.21%. Comparing base (b4a69f6) to head (08755cd).
Report is 88 commits behind head on dev.

Files with missing lines Patch % Lines
editoast/src/views/train_schedule/projection.rs 0.00% 3 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #9661      +/-   ##
==========================================
+ Coverage   37.88%   38.21%   +0.33%     
==========================================
  Files         992      995       +3     
  Lines       90966    91909     +943     
  Branches     1176     1189      +13     
==========================================
+ Hits        34463    35124     +661     
- Misses      56049    56331     +282     
  Partials      454      454              
Flag Coverage Δ
editoast 73.27% <40.00%> (-0.10%) ⬇️
front 20.23% <ø> (+0.11%) ⬆️
gateway 2.18% <ø> (ø)
osrdyne 3.28% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 87.00% <100.00%> (+0.25%) ⬆️

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: update start of free block start in signal projection (space-time chart) when stopping on closed signal core: update start of free block in signal projection (space-time chart) when stopping on closed signal Nov 8, 2024
Copy link
Member

@emersion emersion 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 the frontend (not tested)

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.

Nice improvement, and less dead code if I understand correctly. Also, you will need to rebase because a CI PR happened in between which explains why you see 50 steps (instead of 25) with half of them not running.

@bougue-pe bougue-pe force-pushed the peb/core/signal_projection_closed_signal_stop branch from 6632b0a to 12f144a Compare November 14, 2024 17:20
@Khoyo
Copy link
Contributor

Khoyo commented Nov 15, 2024

(I don't see the dead code removal :p)

I believe we need signal sightings in order to only output green zones for signal that are effectively constraining the projected train. path_signals are about the path that's projected onto, and requirements are zone based, so we cannot only rely on those.

Here is an example:

image

The first green block in this image should not be there, as the first train never sees that signal

@bougue-pe bougue-pe marked this pull request as draft November 20, 2024 12:36
@bougue-pe bougue-pe force-pushed the peb/core/signal_projection_closed_signal_stop branch 2 times, most recently from 56848fe to c5948fa Compare November 21, 2024 17:25
@github-actions github-actions bot added area:core Work on Core Service area:front Work on Standard OSRD Interface modules area:integration-tests Work on Integration test, by nature related to different services labels Nov 21, 2024
@bougue-pe
Copy link
Contributor Author

bougue-pe commented Nov 21, 2024

All rewritten, as discussed with @Khoyo:
Space-requirements start are not displayed in space-time chart, what's displayed for free block is the first moment where a signal must be free (usually signal sighting by the train, or the stop ending when on closed signal).
So:

@bougue-pe bougue-pe marked this pull request as ready for review November 21, 2024 17:45
@bougue-pe bougue-pe force-pushed the peb/core/signal_projection_closed_signal_stop branch from c5948fa to ecce330 Compare November 22, 2024 11:01
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.

Nicely done 👍🏻

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.

Is signal_critical_position better than signal_sighting? I'm sure I don't understand all of it, but intuitively, "signal sighting" means something in my mind (when you see the signal), whereas "signal critical position" feels like it's missing some context: critical for what (or for when)? Feel free to disregard if this is about a shared jargon term.

@bougue-pe bougue-pe force-pushed the peb/core/signal_projection_closed_signal_stop branch from ecce330 to 461433f Compare November 25, 2024 14:51
@bougue-pe bougue-pe requested review from shenriotpro and removed request for bloussou November 26, 2024 08:48
@bougue-pe bougue-pe force-pushed the peb/core/signal_projection_closed_signal_stop branch from 461433f to 34ba402 Compare November 26, 2024 14:20
Field used only on a later call by the signal-projection

Also add integration test on start of space-time chart's free block for stop on closed signal

Signed-off-by: Pierre-Etienne Bougué <[email protected]>
Letting core's API v1 untouched (as it didn't change)

Signed-off-by: Pierre-Etienne Bougué <[email protected]>
@bougue-pe bougue-pe force-pushed the peb/core/signal_projection_closed_signal_stop branch from 34ba402 to 08755cd Compare November 26, 2024 17:23
Copy link
Contributor

@shenriotpro shenriotpro left a comment

Choose a reason for hiding this comment

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

Thanks

@bougue-pe bougue-pe added this pull request to the merge queue Nov 27, 2024
Merged via the queue into dev with commit 8b00ee7 Nov 27, 2024
27 checks passed
@bougue-pe bougue-pe deleted the peb/core/signal_projection_closed_signal_stop branch November 27, 2024 10:28
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:integration-tests Work on Integration test, by nature related to different services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Core: update free block start in space-time chart when stopping on closed-signal (match conflict detection)
6 participants