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

Use consists parameters to train simulation request #9660

Merged
merged 3 commits into from
Jan 9, 2025

Conversation

Wadjetz
Copy link
Member

@Wadjetz Wadjetz commented Nov 8, 2024

fix #9544

@Wadjetz Wadjetz self-assigned this Nov 8, 2024
@Wadjetz Wadjetz added kind:bug Something isn't working area:editoast Work on Editoast Service module:stdcm Short-Term DCM labels 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 91.58879% with 9 lines in your changes missing coverage. Please review.

Project coverage is 81.49%. Comparing base (384dcea) to head (78ec705).
Report is 21 commits behind head on dev.

Files with missing lines Patch % Lines
editoast/src/views/path/pathfinding.rs 65.00% 7 Missing ⚠️
editoast/src/views/timetable/stdcm.rs 92.00% 2 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #9660      +/-   ##
==========================================
- Coverage   81.53%   81.49%   -0.05%     
==========================================
  Files        1059     1061       +2     
  Lines      104491   104609     +118     
  Branches      722      722              
==========================================
+ Hits        85198    85247      +49     
- Misses      19252    19321      +69     
  Partials       41       41              
Flag Coverage Δ
editoast 73.61% <91.42%> (-0.14%) ⬇️
front 89.25% <100.00%> (+<0.01%) ⬆️
gateway 2.18% <ø> (ø)
osrdyne 3.28% <ø> (ø)
railjson_generator 87.50% <ø> (ø)
tests 87.05% <ø> (ø)

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.

@Wadjetz Wadjetz force-pushed the ebe/editoast/stdcm/fix-train-simulation branch from f0f89c1 to e5e6b33 Compare November 8, 2024 15:07
@github-actions github-actions bot removed the area:editoast Work on Editoast Service label Nov 8, 2024
@Wadjetz Wadjetz force-pushed the ebe/editoast/stdcm/fix-train-simulation branch from e5e6b33 to fbf7d12 Compare November 8, 2024 15:11
@Wadjetz Wadjetz force-pushed the ebe/editoast/stdcm/fix-train-simulation branch 4 times, most recently from bedcaf7 to 54c85fb Compare November 20, 2024 13:40
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.

You have three functions (train_simulation_batch, get_train_batch_simulation and consist_train_simulation_batch). Only two of them are required.

  • train_simulation_batch(&[TrainSchedule])
    • Retrieve rolling stocks from rolling_stock_name field`
    • Convert this rolling stocks into PhysicsConsistParameters
    • Call consist_train_simulation_batch
  • consist_train_simulation_batch(&[TrainScheduleWithPhysicsConsist])
    • Check valkey cache
    • Call the pathfinding
    • Cal the simulation
    • Cache the response
    • Return the response

editoast/src/views/train_schedule.rs Outdated Show resolved Hide resolved
editoast/src/views/train_schedule.rs Outdated Show resolved Hide resolved
editoast/src/views/train_schedule.rs Outdated Show resolved Hide resolved
@Wadjetz Wadjetz force-pushed the ebe/editoast/stdcm/fix-train-simulation branch from 54c85fb to 6339879 Compare November 22, 2024 12:20
@github-actions github-actions bot added the area:editoast Work on Editoast Service label Nov 22, 2024
@Wadjetz Wadjetz force-pushed the ebe/editoast/stdcm/fix-train-simulation branch 2 times, most recently from e76a0cc to 463e084 Compare November 25, 2024 09:35
@Wadjetz Wadjetz requested a review from flomonster November 25, 2024 09:37
@Wadjetz Wadjetz force-pushed the ebe/editoast/stdcm/fix-train-simulation branch 2 times, most recently from 9fa4b7f to 4d28984 Compare December 3, 2024 09:31
@Wadjetz Wadjetz requested review from woshilapin and hamz2a December 3, 2024 09:32
@Wadjetz Wadjetz marked this pull request as ready for review December 3, 2024 09:32
@Wadjetz Wadjetz requested a review from a team as a code owner December 3, 2024 09:32
@Wadjetz Wadjetz force-pushed the ebe/editoast/stdcm/fix-train-simulation branch from 4d28984 to c4265fd Compare December 3, 2024 10:01
Copy link
Contributor

@hamz2a hamz2a left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Contributor

@leovalais leovalais left a comment

Choose a reason for hiding this comment

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

Cursory review. I'll come back to this when @flomonster's comments are addressed. Meanwhile, a few comments (mostly about excessive cloning 😄):

(The PR name and commit messages aren't really helpful to understand what's being fixed exactly, and it's not front: commits 😅)

editoast/src/models/fixtures.rs Outdated Show resolved Hide resolved
editoast/src/core/simulation.rs Outdated Show resolved Hide resolved
editoast/src/views/timetable/stdcm.rs Outdated Show resolved Hide resolved
editoast/src/core/simulation.rs Outdated Show resolved Hide resolved
editoast/src/views/timetable/stdcm.rs Outdated Show resolved Hide resolved
editoast/src/views/path/pathfinding.rs Outdated Show resolved Hide resolved
editoast/src/views/path/pathfinding.rs Outdated Show resolved Hide resolved
editoast/src/views/train_schedule.rs Outdated Show resolved Hide resolved
editoast/src/views/train_schedule.rs Outdated Show resolved Hide resolved
editoast/src/views/train_schedule.rs Outdated Show resolved Hide resolved
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.

Thanks.

We need to make sure that the function parameters are as logical and relevant as possible (avoid hacks, as this suits our workflow).

editoast/src/core/simulation.rs Outdated Show resolved Hide resolved
editoast/src/views/path/pathfinding.rs Outdated Show resolved Hide resolved
editoast/src/views/train_schedule.rs Outdated Show resolved Hide resolved
editoast/src/views/timetable/stdcm.rs Outdated Show resolved Hide resolved
@Wadjetz Wadjetz force-pushed the ebe/editoast/stdcm/fix-train-simulation branch from c4265fd to 247fdfa Compare December 3, 2024 13:12
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 left some comments, needs a more thorough review still.

editoast/src/views/path/pathfinding.rs Outdated Show resolved Hide resolved
editoast/src/views/path/pathfinding.rs Outdated Show resolved Hide resolved
@Wadjetz Wadjetz force-pushed the ebe/editoast/stdcm/fix-train-simulation branch from 433f4ab to 4e12429 Compare December 19, 2024 13:13
@Wadjetz Wadjetz requested a review from a team as a code owner December 19, 2024 13:13
@github-actions github-actions bot added the area:front Work on Standard OSRD Interface modules label Dec 19, 2024
@Wadjetz Wadjetz force-pushed the ebe/editoast/stdcm/fix-train-simulation branch 3 times, most recently from bcb07db to db47b16 Compare December 19, 2024 14:13
@Wadjetz Wadjetz requested a review from flomonster December 19, 2024 14:34
Copy link
Contributor

@leovalais leovalais left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes!

editoast/src/views/path/pathfinding.rs Outdated Show resolved Hide resolved
@Wadjetz Wadjetz force-pushed the ebe/editoast/stdcm/fix-train-simulation branch from db47b16 to 53f9ea2 Compare January 2, 2025 09:54
@Wadjetz Wadjetz force-pushed the ebe/editoast/stdcm/fix-train-simulation branch 2 times, most recently from c6b4b58 to 6cc0bc1 Compare January 7, 2025 08:45
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.

LGTM

@Wadjetz Wadjetz force-pushed the ebe/editoast/stdcm/fix-train-simulation branch 3 times, most recently from e5bce37 to 1e9bddc Compare January 8, 2025 13:54
@Maymanaf Maymanaf force-pushed the ebe/editoast/stdcm/fix-train-simulation branch from 1e9bddc to a9cb8c8 Compare January 8, 2025 16:50
@Maymanaf Maymanaf requested a review from a team as a code owner January 8, 2025 16:50
@Maymanaf Maymanaf force-pushed the ebe/editoast/stdcm/fix-train-simulation branch from a9cb8c8 to 9c252d9 Compare January 8, 2025 16:52
@Maymanaf Maymanaf force-pushed the ebe/editoast/stdcm/fix-train-simulation branch from 9c252d9 to 9fe84b8 Compare January 9, 2025 08:50
@Wadjetz Wadjetz enabled auto-merge January 9, 2025 10:07
@Wadjetz Wadjetz added this pull request to the merge queue Jan 9, 2025
Merged via the queue into dev with commit c806a2f Jan 9, 2025
27 checks passed
@Wadjetz Wadjetz deleted the ebe/editoast/stdcm/fix-train-simulation branch January 9, 2025 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:editoast Work on Editoast Service area:front Work on Standard OSRD Interface modules kind:bug Something isn't working module:stdcm Short-Term DCM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stdcm: towed rolling stock parameters aren't used for preprocessing simulations
8 participants