core: compute min between 2 envelope parts and use it in ETCS

[Erashin]

Compute the min curve between 2 envelope parts and use it in ETCS to compute min between indicationSvL and indicationEoA more precisely.
TBD: modify integration test, there should be more precise intersection points between the SvL and EoA.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.60%. Comparing base (574a3e7) to head (69253bf).

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

Additional details and impacted files

@@                       Coverage Diff                        @@
##           alch/core/add_svl_etcs_logic   #10991      +/-   ##
================================================================
- Coverage                         80.64%   80.60%   -0.05%     
================================================================
  Files                              1080     1080              
  Lines                            110825   110762      -63     
  Branches                            745      744       -1     
================================================================
- Hits                              89375    89275     -100     
- Misses                            21407    21444      +37     
  Partials                             43       43              

Flag	Coverage Δ
editoast	72.22% <ø> (-0.19%)	⬇️
front	89.94% <ø> (+<0.01%)	⬆️
gateway	2.18% <ø> (ø)
osrdyne	2.53% <ø> (ø)
railjson_generator	87.58% <ø> (ø)

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.

[eckter]

Nice job

[eckter]

I would have thought it would have been easier to use if it returned the whole range covered by either. And possibly dealing with Envelopes instead of Parts. But you're the caller, you probably know what's most convenient.

[Erashin]

Here were my thoughts on this, tell me what you think about it:

• if by "the whole range covered by either" (probably not what you mean) you mean the whole range, even after the end of one, well the envelope part would have discontinuities, so this isn't doable.
• if by "the whole range covered by either" you mean just the intersecting range of the two curves, then yeah that is a possibility. That would mean moving the part of the code that creates the first non intersecting part of the curve into EtcsBrakingCurves, which is clearly doable.
• for envelopes instead of envelope parts: the pb is how we're going to manage the discontinuities between the successive envelope parts, because they're not necessarily continuous (in our case they are). Except if you have a sol for that, it would work if we assert it is continuous for a min method between 2 Envelopes. The changes would be: putting the current method into a utils, taking positions + speeds as arguments. Call it in both EnvelopePart.min and Envelope.min (2 birds with one stone). EDIT: even then, how would we be managing the different EnvelopeParts' attributes?

[eckter]

We can and do have discontinuities in envelopes, it's just in the final simulation output that they're forbidden.

I meant the whole range, or possibly the whole range given by one of the inputs.

I'm just a little surprised to see a method where we don't really know what range will be covered by the output. Generally speaking we have a reference envelope and we modify parts of it, so having the solution defined on the intersection of two parts feels a little off.

It would probably be easier to understand if you describe where it would be called: what are the two parts that will be used as input? And why are we only interested in the intersection? (and why can't they be envelopes?)

[Erashin]

What I meant by Envelopes with discontinuities not being possible to use the method on is that we can't use interpolateSpeed and intersectStep on them. Imagine envelopeA going from a constant speed of 100 to a constant speed of 50, and envelopeB being at 75 at the time: how do we manage the intersection between the 2 curves? Would you just cut it off, create a new EnvelopePart starting at this discontinuity? How do we manage this without the method having quite a few specific cases and ending up being quite complex?

The 2 parts used as input are just 2 random parts in my head, except they have to have a common range of position because otherwise how would we determine which of them is the min? Only alternative i see would be to return an Envelope combining them both in that case, but that would mean the method shouldn't be called min, but probably something like mergeMin or something. And it would also mean the output can be discontinuous, which leads to the following point.

Why are we only interested in the intersection: the objective of this method, and also the reason why I'm working with EnvelopeParts rather than Envelopes, is to keep the continuity property of the EnvelopePart: the output will be continuous (which is handy when creating braking curves). The only way to have the output be continuous is:

• taking the min of the 2 curves at least in the position-range intersection
• following the curve before the intersection if it is the min at the start of the intersection
• following the curve after the intersection if it is the min at the end of the intersection (which i could have coded actually)

However, as you've said, a good alternative would be to have the position-range given as an input: only pb would be that, if the output is still an EnvelopePart, you'd have cases when the output's range would be smaller than the input range for continuity reasons. Which i think is ok-ish kinda.

[eckter]

So I feel like we're kind of dodging some important questions by only returning the intersection. The questions being what we're going to do at those borders. I wouldn't want to have the caller of this method reassemble some of the input parts to build a consistent final result, this method wouldn't be usable on its own.

What I'm really uncomfortable about is not really knowing the "domain" of the output. We expect a set of points but without knowing where they'll be, it feels like we'd need a lot of weird post-processing (and pre-processing to even have a valid overlap).

I'd really like if we could:

1. have a clear output domain that can't be ill-defined (such as "all of the range covered by the leftmost parameter")
2. Handle discontinuities in this method, or possibly in an auxiliary one. I don't think handling envelope parts really helps here.

Actually we've had very similar use cases with mareco. Generally we tolerate discontinuities during intermediate steps, and then add accelerations or decelerations to fix them. But we need to plan carefully to avoid having weird errors or ill-defined results, the mareco module took a lot of rewrites (and isn't even stable today).

---

I know I'm not really helping much here, I don't have any clear path to offer. If possible I'd like some fresh eyes (@bougue-pe 👀). Just to be clear: I'm not strongly against the current solution, just a little afraid of what's coming next

[bougue-pe]

😅 This is a bit tough, I hope I understood the stakes.

The first use-case:

• computing the min of 2 braking curves that are continuous, monotonous (maybe not strictly if we consider the maintain-speed part of SvL).
• the result is expected to start from the [min(beginPositions); maxSpeed] point (both curves start -or can start- at maxSpeed).
• the end of the result is debatable:
  • can finish at min(endPositions), then external work to add final parts: maintain-speed to reach second curve, then use the second curve to its endPosition.
  • can finish at max(endPositions): internally add a maintain-speed part to link to second curve when min(endPositions) is reached.

I see 4 valid options here (detailed by my order of preference).

Important

TLDR

• Working on Envelopes (and assume discontinuities) has my preference (on the expected RoI also).
• We can easily manage continuity and size of position-ranges for the described case by adding missing parts, so any solution looks valid.

1 - work on Envelopes (force one intersection, return union of position ranges)

That would mean assuming discontinuities sometimes, indeed.
But forcing monotonous (and in the same direction) inputs guarantees monotonous output on the intersecting range.
And forcing continuous inputs guarantees continuous output on the intersecting range.
Monotony and/or continuity are quite easy to obtain on the first use-case we have (adding constant parts at the beginning and the end of curves so that their position range is equal), so it wouldn't be a major issue.

That would also mean applying attributes given in inputs, which is better IMO (allows for finer result).

That would require a clear choice of what attributes are applied when strictly equal (the previous min curve, then the next min curve, then the leftmost param if no previous?).

That means that going back to working on EnvelopePart is a bit tricky if required (converting Envelope slice to a Part while checking that Part requirements are respected).

That may require some kind of "final cleanup" (optional?) to regroup parts that are "identical" to obtain a clean single braking curve when possible (in our use case, it is not mandatory to avoid mutliple consecutive braking parts in a single ETCS braking, is it?).

That may require a bit more work to implement and to fit it in current code.

From the point-of-view of an external user of this method, I think we would have more use of this: it can more safely be used to add braking and acceleration curves (the extra cost on perf may be an issue?). Superposing them blindly would just work, and the code would be much clearer IMO.

2 - work on EnvelopeParts and return a union of position ranges (force one intersection, identical monotony)

TBD if the result should be an Envelope (my preference) or an EnvelopePart, see below.

The logic of the method would actually be a bit "reversed": return a curve that's the min position for all the speed range, and keep the min speed when 2 positions lead to different speed.

That would also mean assuming discontinuities sometimes (but it would not happen for the first use-case).
That would mean adding a maintain-speed piece:

• either a point to the EnvelopePart result if allowed (post-processing to change it to a separate EnvelopePart?)
• or just a dedicated EnvelopePart to the Envelope result (better IMO)

The amount of work is OK I think to have it fit in the current code.

Less external "unclear" code to assemble parts, the logic is contained and a bit more easy to explain IMO.

3 - Keep the current code

Staying with the current proposition is OK IMO, but we should probably move it back to the ETCSBrakingCurve.kt to avoid any misuse, as I agree it's a bit of a trap (for 2 acceleration curves, the top part would be broken).

Or keep it here, add asserts on braking and TODO for the rest to ensure future mutualization.

4 - work on EnvelopeParts and return intersection of position ranges (force one intersection, continuity, identical monotony)

This guarantees a continuous and monotonous output.
We may also force identical attributes of inputs to use the same one in output?
In the first use-case, we could add an extra first-point (pos: 0, speed: beginSpeed) to make sure that intersection is enough, I think.

This would work for our first case, cover the concern on the output position-range and the amount of work is OK I guess.

It would keep the "unclear" external code that's assembling the parts.

[eckter]

I feel like we should really convert that file to kotlin if we write that kind of stuff 😄

[Erashin]

I was actually in the midst of doing it and there were a few pbs while i was converting it. I'll give it another shot though, we both agree it's better.

[eckter]

Suggested change

	TreeSet<Double> keyPositions =
	Arrays.stream(envelopePartA.positions).boxed().collect(Collectors.toCollection(TreeSet::new));
	keyPositions.addAll(Arrays.stream(envelopePartB.positions).boxed().collect(Collectors.toSet()));
	keyPositions = new TreeSet<>(keyPositions.subSet(start, true, end, true));
	NavigableSet<Double> keyPositions =
	Arrays.stream(envelopePartA.positions).boxed().collect(Collectors.toCollection(TreeSet::new));
	keyPositions.addAll(Arrays.stream(envelopePartB.positions).boxed().collect(Collectors.toSet()));
	keyPositions = keyPositions.subSet(start, true, end, true);

Avoids instantiating a new TreeSet from the subset

[bougue-pe]

This looks bugged, no?
I would expect the following results (easy to check for 5 and 15):

Suggested change

	new double[] {0, 5, 10, 14, 15, 16},
	new double[] {100, 79.05694150420949, 50, 38.72983346207417, 35.35533905932738, 0}),
	new double[] {0, 5, 10, 14, 15, 16},
	new double[] {100, 75, 50, 30, 25, 0}),

[bougue-pe]

(if keeping the current behaviour) If speeds are equal at the beginning of pos-intersection, we probably want the min(beginPosA, beginPosB) (or max).
Otherwise, the result won't be identical if we exchange envelopePartA and envelopePartB.

[bougue-pe]

Unexpected output here too, if using min(beginPositions) in case of equality (both speeds are 100 at position 5):

Suggested change

	new double[] {5, 7.142857142857143, 10, 14, 15, 16},
	new double[] {100, 90.63269671749657, 50, 38.72983346207417, 35.35533905932738, 0}));
	new double[] {0, 5, 10, 14, 15, 16},
	new double[] {150, 100, 50, 30, 25, 0}));