nilsbeck/add stop balancing #67
Conversation
model_objective_stop_balancing.go
Outdated
| move Move, | ||
| ) float64 { | ||
| solution := move.Solution() | ||
| oldMax := t.Max(solution, nil) |
There was a problem hiding this comment.
I think you should compute the maximum and minimum in a single for loop - better for performance
model_objective_stop_balancing.go
Outdated
| return float64(t.Max(solution, nil)) | ||
| } | ||
|
|
||
| func (t *balanceObjectiveImpl) Max(solution Solution, move SolutionMoveStops) int { |
There was a problem hiding this comment.
Max could also be a normal, unexported function function
There was a problem hiding this comment.
renamed it to MaxStops
|
|
||
| // NewStopBalanceObjective returns a new StopBalanceObjective. | ||
| func NewStopBalanceObjective() ModelObjective { | ||
| return &balanceObjectiveImpl{} |
There was a problem hiding this comment.
Good question, just followed the same structure as for the other objectives. I have no better answer than that :(
There was a problem hiding this comment.
I looked at a few of the structures in here some time ago and it may benefit from a deeper consideration and exploration of when to use pointers versus when not to. @nilsbeck my opinion is to maintain consistency for now and we should create an issue to possibly refactor later.
model_objective_stop_balancing.go
Outdated
| vehicle = move.Vehicle() | ||
| } | ||
|
|
||
| for _, v := range solution.Vehicles() { |
There was a problem hiding this comment.
Does solution.Vehicles() allocate a slice? You should check that and try to avoid it if possible
There was a problem hiding this comment.
I access the slice of vehicles directly now. before, there was slice cloning involved.
model_objective_stop_balancing.go
Outdated
| } | ||
|
|
||
| for _, v := range solution.Vehicles() { | ||
| if max < v.NumberOfStops() { |
There was a problem hiding this comment.
you repeat function calls often like v.NumberOfStops(). It's not always the case that the go compiler optimises that away. Better to assign v.NumberOfStops() to a variable and reuse that one. Same with v.Index() and move.StopPositionsLength().
| maxBefore := 0 | ||
| moveExists := move != nil | ||
| var vehicle SolutionVehicle | ||
| if moveExists { |
There was a problem hiding this comment.
moveExisits is always true.
There was a problem hiding this comment.
the function is used in the Value calculation without a move (because there only is a solution then). so it can be nil
maxBefore, _ := t.MaxStops(solution, nil)
model_objective_stop_balancing.go
Outdated
| return float64(maxBefore) | ||
| } | ||
|
|
||
| func (t *balanceObjectiveImpl) MaxStops(solution Solution, move SolutionMoveStops) (int, int) { |
There was a problem hiding this comment.
Why is MaxStops an exported function?
There was a problem hiding this comment.
made it not exported. was at the time because I was thinking of a unit test. but left the test because it seems unnecessary.
| "lon": -78.828547, | ||
| "lat": 35.962635 | ||
| }, | ||
| "precedes": ["s16", "s23"] |
There was a problem hiding this comment.
Why is there only one stop that precedes others? 🤔
There was a problem hiding this comment.
I copied the stops from another tests and apparently forgot to remove one of the precedes...removed it.
There was a problem hiding this comment.
Ugh, yeah. There is no way currently to provide options for a golden file test right now. I noticed that we also miss golden file tests for clustering objective / constraint and such.
Can / should we combine the options with the existing golden file testing? I could imagine having another optional file next to the input file that defines the options. 🤔
Anyway, probably not for this PR, but potentially a fast-follow one. Otherwise, I feel like we will inflate our testing considerably and we are also missing some tests for cluster objective and such. 😊
Description
This PR adds a new objective to balance the number of stops on vehicles. It can be turned on by setting the instance option to a value > 0:
"-model.objectives.stopbalance", "1000.0"or in the model code once the options are passed, before creating the model:
options.Model.Objectives.StopBalance = 1000.0The value given is used to penalize the imbalance.