-
Notifications
You must be signed in to change notification settings - Fork 45
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
Refine allowance tolerances #6404
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #6404 +/- ##
=============================================
- Coverage 79.55% 28.01% -51.54%
- Complexity 2169 2170 +1
=============================================
Files 338 1032 +694
Lines 12796 127750 +114954
Branches 1775 2603 +828
=============================================
+ Hits 10180 35793 +25613
- Misses 1925 90442 +88517
- Partials 691 1515 +824
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
3a54198
to
e47c3bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New tests look great ! The work with tolerences as well. I'm just not quite sure what to do with the sorting of the ranges that makes the computation sub-optimal in terms of energy consumption. We should probably talk about it.
...lope-sim/src/main/java/fr/sncf/osrd/envelope_sim/allowances/AbstractAllowanceWithRanges.java
Show resolved
Hide resolved
...lope-sim/src/main/java/fr/sncf/osrd/envelope_sim/allowances/AbstractAllowanceWithRanges.java
Outdated
Show resolved
Hide resolved
...lope-sim/src/main/java/fr/sncf/osrd/envelope_sim/allowances/AbstractAllowanceWithRanges.java
Outdated
Show resolved
Hide resolved
core/envelope-sim/src/test/java/fr/sncf/osrd/envelope_sim/AllowanceRangesTests.java
Outdated
Show resolved
Hide resolved
core/src/test/kotlin/fr/sncf/osrd/stdcm/StandardAllowanceTests.kt
Outdated
Show resolved
Hide resolved
...lope-sim/src/main/java/fr/sncf/osrd/envelope_sim/allowances/AbstractAllowanceWithRanges.java
Outdated
Show resolved
Hide resolved
core/envelope-sim/src/test/java/fr/sncf/osrd/envelope_sim/AllowanceRangesTests.java
Outdated
Show resolved
Hide resolved
core/src/test/kotlin/fr/sncf/osrd/stdcm/StandardAllowanceTests.kt
Outdated
Show resolved
Hide resolved
68b8223
to
7cc603e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📈
7cc603e
to
2eebe3d
Compare
instead of among ranges Also, change order in which allowance ranges are computed start with the shortest ranges instead of the ones with the lowest value
2eebe3d
to
8eb223e
Compare
In this PR I offer two changes:
The first commit changes two tests that highlights why I offer to change those behaviors (a test with a short allowance range and a very bug value will fail, and a test with many stops on a path, on top of an allowance accumulates approximation errors)
The next two fix those problems
The last one updates all tests regarding allowances to change the tolerances in
assertEquals
to be more restrictiveAll of those are not atomic, so I'll squash them before merging
EDIT: distributing allowance tolerance according to section relative time makes for a more accurate computation but will make some particular allowances fail (since we'll demand high precision on small ranges (thus a small range with very high allowance value and very high imposed speed will fail). If that's not okay, we may distribute tolerance with a function that favors small values)