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

Move EnvelopeSimContext out of Allowances initialization #3529

Merged
merged 1 commit into from
Mar 14, 2023

Conversation

Castavo
Copy link
Contributor

@Castavo Castavo commented Mar 14, 2023

Doing this refactor has several advantages:

  • We avoid duplication of the context (namely in RJSStandaloneTrainScheduleParser), since we always build a EnvelopeSimContext in StandaloneSim.run .
  • With power restrictions, EnvelopeSimContext init is also going to get slower, so avoid duplication has twice the benefits.
  • The context isn't semantically linked to the Allowance classes.
  • EnvelopeSimContext initialisation is going to get a bit more complex, so avoiding to maintain a custom building method is preferable

@Castavo Castavo requested a review from a team as a code owner March 14, 2023 09:33
@codecov
Copy link

codecov bot commented Mar 14, 2023

Codecov Report

Merging #3529 (39582cf) into dev (8705e14) will decrease coverage by 0.01%.
The diff coverage is 96.15%.

@@             Coverage Diff              @@
##                dev    #3529      +/-   ##
============================================
- Coverage     68.20%   68.20%   -0.01%     
+ Complexity     1897     1896       -1     
============================================
  Files           411      411              
  Lines         20443    20438       -5     
  Branches       1519     1519              
============================================
- Hits          13943    13939       -4     
  Misses         5798     5798              
+ Partials        702      701       -1     
Flag Coverage Δ
core 79.17% <96.15%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
.../fr/sncf/osrd/cli/StandaloneSimulationCommand.java 0.00% <0.00%> (ø)
...pe_sim/allowances/AbstractAllowanceWithRanges.java 89.82% <100.00%> (-0.05%) ⬇️
.../osrd/envelope_sim/allowances/LinearAllowance.java 100.00% <100.00%> (ø)
.../osrd/envelope_sim/allowances/MarecoAllowance.java 97.14% <100.00%> (ø)
...fr/sncf/osrd/api/StandaloneSimulationEndpoint.java 93.93% <100.00%> (ø)
.../envelope_sim_infra/EnvelopeSimContextBuilder.java 75.00% <100.00%> (+8.33%) ⬆️
...ljson/parser/RJSStandaloneTrainScheduleParser.java 68.05% <100.00%> (-0.87%) ⬇️
...ava/fr/sncf/osrd/standalone_sim/StandaloneSim.java 90.90% <100.00%> (ø)
...ava/fr/sncf/osrd/stdcm/graph/STDCMSimulations.java 97.50% <100.00%> (ø)
.../sncf/osrd/stdcm/graph/STDCMStandardAllowance.java 76.31% <100.00%> (ø)

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

@Castavo
Copy link
Contributor Author

Castavo commented Mar 14, 2023

The coverage goes down only because I removed lines that were tested, I did not add untested lines

@Castavo Castavo requested a review from eckter March 14, 2023 09:42
@Castavo Castavo merged commit 351dbeb into dev Mar 14, 2023
@Castavo Castavo deleted the bpt/move-context-out-of-allowances branch March 14, 2023 10:05
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.

2 participants