Provide rest timewindows compatible with vehicle

[fonsecadeline]

No description provided.

[senhalil]

Does or-tools only have a problem when a single rest TW doesn't overlap with vehicle.tw or when there is no feasible TW for the pause that overlaps with the vehicle TW? Where the segfault/error comes from inside or-tools or from our code?

tsp_simple.cc has the following code which should prevent such issues inside or-tools normally. if the error is in our cpp code, we can correct it there.

    for (const TSPTWDataDT::Rest& rest : vehicle.Rests()) {
      IntervalVar* const rest_interval = solver->MakeFixedDurationIntervalVar(
          std::max(rest.ready_time[0], vehicle.time_start),
          std::min(rest.due_time[0], vehicle.time_end - rest.service_time),
          rest.service_time, // Currently only one timewindow
          false, absl::StrCat("Rest/", rest.rest_id, "/", vehicle_index));
      rest_array.push_back(rest_interval);

optimiser-ortool model should be able to handle multiple TWs for the rests (even if some not overlapping with the vehicle's TW) because as long as there is an overlapping rest TW, the model/vehicle is feasible. However, if there is no overlapping rest TW then the model/vehicle itself is infeasible and for such instances, this code would ignore all the TWs for the rest, if I am not mistaken. (Is that it?)

Instead of ignoring, I suggest we refuse incoherent vehicles/instances (because if you ask for a 10-minute pause between 12 and 15 but if the vehicle works between 16 to 20 then something is not right, right?)

[fonsecadeline]

Does or-tools only have a problem when a single rest TW doesn't overlap with vehicle.tw or when there is no feasible TW for the pause that overlaps with the vehicle TW? Where the segfault/error comes from inside or-tools or from our code?

tsp_simple.cc has the following code which should prevent such issues inside or-tools normally. if the error is in our cpp code, we can correct it there.

  for (const TSPTWDataDT::Rest& rest : vehicle.Rests()) {
    interval_vars.emplace_back(solver.MakeNumVar(
        std::max(rest.ready_time[0], vehicle.time_start),
        std::min(rest.due_time[0], vehicle.time_end - rest.service_time),
        absl::StrCat("Rest/", rest.rest_id, "/", vehicle_index)));
  }

optimiser-ortool model should be able to handle multiple TWs for the rests (even if some not overlapping with the vehicle's TW) because as long as there is an overlapping rest TW, the model/vehicle is feasible. However, if there is no overlapping rest TW then the model/vehicle itself is infeasible and for such instances, this code would ignore all the TWs for the rest, if I am not mistaken. (Is that it?)

Instead of ignoring, I suggest we refuse incoherent vehicles/instances (because if you ask for a 10-minute pause between 12 and 15 but if the vehicle works between 16 to 20 then something is not right, right?)

@senhalil In this particular case, only 1/7 of rest's timewindows overlapped vehicle timewindows. What we are trying to fix here is the fact that we return a solution with 1/1 unassigned if we provide all rests timewindows to ortools (segfault happened in the case where no rest timewindow was provided).

Your comment pointed out that in the case were none of rest's timewindows are compatible with vehicle ones, then we might violated rest timewindows.

[senhalil]

In this particular case, only 1/7 of rest's timewindows overlapped vehicle timewindows. What we are trying to fix here is the fact that we return a solution with 1/1 unassigned if we provide all rests timewindows to ortools

I don't understand why this is happening. Or-tools should simply ignore the pause "intervals" (timewindows) that doesn't coincide with the vehicle, intriguing..

Hmm I just found it during digging further, I think. Optimizer-ortools implements the pause for one and only one TW, when there are multiple TW's all except the first are ignored. // Currently only one timewindow Because of this when the first TW is the "incoherent" TW, pause is irrealisable, which leads to all services to be unplanned.

So long story short, we don't actually support multiple TWs for pauses. Apparently, it is already not possible send multiple-TWs for pauses via mt-web, so we can add a validator which verifies that there is only one TW for pauses (and we can add a note on vrp_input.rb that currently only one TW is supported for pause).

(segfault happened in the case where no rest timewindow was provided)

I think the segfault happens (when there is no tw for pause) because even if there is no TW, in the above code snippet I shared we call rest.ready_time[0] and rest.due_time[0] which is not initialized (in https://github.com/Mapotempo/optimizer-ortools/blob/f79803e51ca31a65fbc2e73720b0f10879d00252/tsptw_data_dt.h#L951-L965 ) so when or-tools tries to use the reference, it crashes. We need something as follows before the v->rests.emplace_back call:

      if (timewindows.empty()) {
        ready_time.push_back(0);
        due_time.push_back(CUSTOM_MAX_INT);
        tws_counter_ += 1;
      }

Your comment pointed out that in the case were none of rest's timewindows are compatible with vehicle ones, then we might violated rest timewindows.

Lets add a check for this case at the validation stage so that we don't accept incoherent pause/vehicle definitions. What do you think?

[fonsecadeline]

In this particular case, only 1/7 of rest's timewindows overlapped vehicle timewindows. What we are trying to fix here is the fact that we return a solution with 1/1 unassigned if we provide all rests timewindows to ortools

I don't understand why this is happening. Or-tools should simply ignore the pause "intervals" (timewindows) that doesn't coincide with the vehicle, intriguing..

Hmm I just found it during digging further, I think. Optimizer-ortools implements the pause for one and only one TW, when there are multiple TW's all except the first are ignored. // Currently only one timewindow Because of this when the first TW is the "incoherent" TW, pause is irrealisable, which leads to all services to be unplanned.

So long story short, we don't actually support multiple TWs for pauses. Apparently, it is already not possible send multiple-TWs for pauses via mt-web, so we can add a validator which verifies that there is only one TW for pauses (and we can add a note on vrp_input.rb that currently only one TW is supported for pause).

(segfault happened in the case where no rest timewindow was provided)

I think the segfault happens (when there is no tw for pause) because even if there is no TW, in the above code snippet I shared we call rest.ready_time[0] and rest.due_time[0] which is not initialized (in https://github.com/Mapotempo/optimizer-ortools/blob/f79803e51ca31a65fbc2e73720b0f10879d00252/tsptw_data_dt.h#L951-L965 ) so when or-tools tries to use the reference, it crashes. We need something as follows before the v->rests.emplace_back call:

      if (timewindows.empty()) {
        ready_time.push_back(0);
        due_time.push_back(CUSTOM_MAX_INT);
        tws_counter_ += 1;
      }

Your comment pointed out that in the case were none of rest's timewindows are compatible with vehicle ones, then we might violated rest timewindows.

Lets add a check for this case at the validation stage so that we don't accept incoherent pause/vehicle definitions. What do you think?

I will rework this PR with these comments. We do need a validation, but we should also edit periodic_visits.rb in order to only provide one timewindow for every generated rest. In other words, my edit in ortools.rb should rather be in periodic_visits.rb.

[fonsecadeline]

Test cassé est un des tests aléatoires corrigé par Mapotempo/optimizer-ortools#27 : SplitClusteringTest#test_avoid_capacities_overlap

[senhalil]

Test cassé est un des tests aléatoires corrigé par Mapotempo/optimizer-ortools#27 SplitClusteringTest#test_avoid_capacities_overlap

I don't see how Mapotempo/optimizer-ortools#27 can fix a flaky optim-api test. Copy/paste typo? Did you mean #282?

[fonsecadeline]

Test cassé est un des tests aléatoires corrigé par Mapotempo/optimizer-ortools#27 SplitClusteringTest#test_avoid_capacities_overlap

I don't see how Mapotempo/optimizer-ortools#27 can fix a flaky optim-api test. Copy/paste typo? Did you mean #282?

Maybe I mixed PRs, you pushed a list of tests that break randomly. I figured that was the PR that fixed it.

[senhalil]

Maybe I mixed PRs, you pushed a list of tests that break randomly. I figured that was the PR that fixed it.

Yeah, the list is introduced in 3941e44. optim-ortools#27 is not in this repo

[senhalil]

I think doc needs clarification and a check (and its test) need modification.

[senhalil]

There might be an issue with the

[senhalil]

@braktar is there any behavior changes here? Do we need to to launch the benchmarks of scheduling? If not, 🚀

[braktar]

There is 2 fixes in context of scheduling : Rests now have a correct timewindow according to the vehicle. And we now allow multiple rests with the same day index.