Fix unplanning bug for plan one of plan unit

[sorig]

Context

I spoke to @nmisek earlier this week about a bug we discovered when implementing a custom routing model. I thought it would be easiest to show you the issue with a failing test + a potential fix that we have been using internally.

The easiest way to understand what I'm talking about is to paste my updated unit tests into the develop branch and see them fail. I might have misunderstood the intent of the code but hopefully you will agree with the tests.

Problem description

While using PlanOneOfPlanUnits to implement a disjunctive constraint on (pickup, dropoff) stop pairs, we found that the solver was giving very weird solutions. I debugged the issue and found out that the solver was unable to correctly unplan stops that were part of disjunctive constraints.

I found the cause of this issue in bestMovePlanOneOfUnit. The original implementation was:

func (v SolutionVehicle) bestMovePlanOneOfUnit(
	ctx context.Context,
	planUnit *solutionPlanUnitsUnitImpl,
) SolutionMove {
	move := NotExecutableMove

	for _, planUnit := range planUnit.solutionPlanUnits {
		move = move.TakeBest(
			v.BestMove(ctx, planUnit),
		)
	}
	return move
}

The function returns a move associated with a StopPlanUnit (assuming no nesting of PlanOneOfPlanUnits) instead of the PlanOneOfPlanUnit that is passed to the function. When the move is executed, stops are attached to the solution but the bookkeeping of planned / unplanned PlanUnits is not done correctly. Hence you end up with stops attached to solutions, but the disjunctive plan unit is marked as unplanned (see test "TestPlanUnitsUnit/PlanOneOfPlanUnits"). This explains why we observed that the solver was unable to unplan the stops.

When I was writing up this PR and the tests for this problem, I also discovered that the same bookkeeping is wrong for nested PlanOneOfPlanUnit (see test: "TestModel_NewPlanOneOfPlanUnits"). I discovered this because the existing tests include these nested structures but they didn't catch the unplanned/planned weirdness. We don't use nested disjunctive constraints, but I wanted to point this out to you here as well.

Fix

The solution to our core problem was to return a corrected move in bestMovePlanOneOfUnit

return newSolutionMoveUnits(planUnit, SolutionMoves{move})

and removing a constraint inside newSolutionMoveUnits()

The rest of the code is related to fixing the planned / unplanned bookkeeping of nested PlanOneOfPlanUnits.

[sorig]

I believe this bug affects your alternate stops feature. We haven't used that feature so I haven't tested how this changes alternate stops.

[nmisek]

Thank you @sorig - looping in @merschformann or @dirkschumacher for a look here.

[merschformann]

Thank you for the report @sorig !

I can see that the change breaks the stop group and alternate features. 🤔
I think I am following your concern though. We will take a look at it tomorrow.

Thanks again for the feedback! 🤗

[sorig]

Any updates on this?

[merschformann]

Any updates on this?

Hey @sorig !

Sorry for taking so long to circle back here. We looked into this, but weren't able to fully reproduce the issue you are seeing. So, we need to plan for more time to spend on analyzing it, which has been difficult.

Maybe you have some more info for us to better isolate and reproduce the issue. Also, can you elaborate a bit on "disjunctive constraints"?

Again, sorry for taking our time here. ❤️

Kind regards,
Marius